January 2021 - Alsa-devel - mailman.alsa-project.org

[PATCH 1/2] ASoC: qcom: lpass: Fix hardcoded SC7810 DAI IDs
by Stephan Gerhold 15 Jan '21

15 Jan '21

LPASS is broken on MSM8916/DragonBoard 410c since Linux 5.10. Attempting to play HDMI audio results in: ADV7533: lpass_platform_pcmops_hw_params: invalid interface: 3 ADV7533: lpass_platform_pcmops_trigger: invalid 3 interface apq8016-lpass-cpu 7708000.audio-controller: ASoC: error at soc_component_trigger on 7708000.audio-controller: -22 Attempting to record analog audio results in: Unable to handle kernel NULL pointer dereference at virtual address 00000000000001e4 Internal error: Oops: 96000004 [#1] PREEMPT SMP CPU: 1 PID: 1568 Comm: arecord Not tainted 5.11.0-rc3 #20 Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) pc : regmap_write+0x14/0x80 lr : lpass_platform_pcmops_open+0xd8/0x210 [snd_soc_lpass_platform] Call trace: regmap_write+0x14/0x80 lpass_platform_pcmops_open+0xd8/0x210 [snd_soc_lpass_platform] snd_soc_component_open+0x2c/0x94 ... The problem was introduced in commit 7cb37b7bd0d3 ("ASoC: qcom: Add support for lpass hdmi driver"). The mistake made there is that lpass.h now contains #include <dt-bindings/sound/sc7180-lpass.h> and then the SC7810 DAI IDs are hardcoded all over lpass-cpu.c and lpass-platform.c. But apq8016 and ipq806x have completely different DAI IDs so now MI2S_QUATERNARY (HDMI) is invalid and MI2S_TERTIARY (= LPASS_DP_RX in sc7180-lpass.h) is treated as HDMI port. Therefore, LPASS is broken on all platforms except SC7810. This commit fixes the problem by removing the include from lpass.h. To check if a DAI is an HDMI port, we compare the dai_id with a new "hdmi_dai" variable in lpass_variant. For SC7180 this is set to LPASS_DP_RX (as before), for all other platform to some unrealistic value (UINT_MAX). Cc: Srinivasa Rao Mandadapu <srivasam(a)codeaurora.org> Cc: Srinivas Kandagatla <srinivas.kandagatla(a)linaro.org> Fixes: 7cb37b7bd0d3 ("ASoC: qcom: Add support for lpass hdmi driver") Signed-off-by: Stephan Gerhold <stephan(a)gerhold.net> --- Srinivas mentioned a potential different fix here: https://lore.kernel.org/alsa-devel/4b34bd4f-e7bc-84f9-5e8a-b2348c17b7aa@lin… Instead of this patch, we could change the dt-bindings for LPASS, so that all SoCs use consistent DAI IDs. In general, I think this might be cleaner, especially in case more special DAIs are added in the future. However, when I made this patch (before Srinivas mentioned it) I tried to avoid changing the dt-bindings because: - Changing dt-bindings after they are added is generally discouraged. but more importantly: - lpass-ipq806x.c does not seem to have PRIMARY, SECONDARY, ... but something completely different. I know nothing about that platform so I don't know how to handle it. --- sound/soc/qcom/lpass-apq8016.c | 1 + sound/soc/qcom/lpass-cpu.c | 9 ++-- sound/soc/qcom/lpass-ipq806x.c | 1 + sound/soc/qcom/lpass-lpaif-reg.h | 2 +- sound/soc/qcom/lpass-platform.c | 77 +++++++++++--------------------- sound/soc/qcom/lpass-sc7180.c | 1 + sound/soc/qcom/lpass.h | 4 +- 7 files changed, 39 insertions(+), 56 deletions(-) diff --git a/sound/soc/qcom/lpass-apq8016.c b/sound/soc/qcom/lpass-apq8016.c index 8507ef8f6679..40e9c66a74a9 100644 --- a/sound/soc/qcom/lpass-apq8016.c +++ b/sound/soc/qcom/lpass-apq8016.c @@ -273,6 +273,7 @@ static struct lpass_variant apq8016_data = { .num_clks = 2, .dai_driver = apq8016_lpass_cpu_dai_driver, .num_dai = ARRAY_SIZE(apq8016_lpass_cpu_dai_driver), + .hdmi_dai = LPASS_HDMI_DAI_UNSUPPORTED, .dai_osr_clk_names = (const char *[]) { "mi2s-osr-clk0", "mi2s-osr-clk1", diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c index c5e99c2d89c7..b22992050646 100644 --- a/sound/soc/qcom/lpass-cpu.c +++ b/sound/soc/qcom/lpass-cpu.c @@ -707,7 +707,8 @@ static unsigned int of_lpass_cpu_parse_sd_lines(struct device *dev, } static void of_lpass_cpu_parse_dai_data(struct device *dev, - struct lpass_data *data) + struct lpass_data *data, + struct lpass_variant *variant) { struct device_node *node; int ret, id; @@ -724,7 +725,7 @@ static void of_lpass_cpu_parse_dai_data(struct device *dev, dev_err(dev, "valid dai id not found: %d\n", ret); continue; } - if (id == LPASS_DP_RX) { + if (id == variant->hdmi_dai) { data->hdmi_port_enable = 1; dev_err(dev, "HDMI Port is enabled: %d\n", id); } else { @@ -766,7 +767,7 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev) drvdata->variant = (struct lpass_variant *)match->data; variant = drvdata->variant; - of_lpass_cpu_parse_dai_data(dev, drvdata); + of_lpass_cpu_parse_dai_data(dev, drvdata, variant); res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpass-lpaif"); @@ -820,7 +821,7 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev) for (i = 0; i < variant->num_dai; i++) { dai_id = variant->dai_driver[i].id; - if (dai_id == LPASS_DP_RX) + if (dai_id == variant->hdmi_dai) continue; drvdata->mi2s_osr_clk[dai_id] = devm_clk_get(dev, diff --git a/sound/soc/qcom/lpass-ipq806x.c b/sound/soc/qcom/lpass-ipq806x.c index 92f98b4df47f..a0c7c5eb03f1 100644 --- a/sound/soc/qcom/lpass-ipq806x.c +++ b/sound/soc/qcom/lpass-ipq806x.c @@ -149,6 +149,7 @@ static struct lpass_variant ipq806x_data = { .dai_driver = &ipq806x_lpass_cpu_dai_driver, .num_dai = 1, + .hdmi_dai = LPASS_HDMI_DAI_UNSUPPORTED, .dai_osr_clk_names = (const char *[]) { "mi2s-osr-clk", }, diff --git a/sound/soc/qcom/lpass-lpaif-reg.h b/sound/soc/qcom/lpass-lpaif-reg.h index 405542832e99..332f1b9ba674 100644 --- a/sound/soc/qcom/lpass-lpaif-reg.h +++ b/sound/soc/qcom/lpass-lpaif-reg.h @@ -133,7 +133,7 @@ #define LPAIF_WRDMAPERCNT_REG(v, chan) LPAIF_WRDMA_REG_ADDR(v, 0x14, (chan)) #define LPAIF_INTFDMA_REG(v, chan, reg, dai_id) \ - ((v->dai_driver[dai_id].id == LPASS_DP_RX) ? \ + ((v->dai_driver[dai_id].id == v->hdmi_dai) ? \ LPAIF_HDMI_RDMA##reg##_REG(v, chan) : \ LPAIF_RDMA##reg##_REG(v, chan)) diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c index d1c248590f3a..140609fe835d 100644 --- a/sound/soc/qcom/lpass-platform.c +++ b/sound/soc/qcom/lpass-platform.c @@ -128,7 +128,7 @@ static int lpass_platform_pcmops_open(struct snd_soc_component *component, return dma_ch; } - if (cpu_dai->driver->id == LPASS_DP_RX) { + if (cpu_dai->driver->id == v->hdmi_dai) { map = drvdata->hdmiif_map; drvdata->hdmi_substream[dma_ch] = substream; } else { @@ -173,7 +173,7 @@ static int lpass_platform_pcmops_close(struct snd_soc_component *component, unsigned int dai_id = cpu_dai->driver->id; data = runtime->private_data; - if (dai_id == LPASS_DP_RX) + if (dai_id == v->hdmi_dai) drvdata->hdmi_substream[data->dma_ch] = NULL; else drvdata->substream[data->dma_ch] = NULL; @@ -205,7 +205,7 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, if (dir == SNDRV_PCM_STREAM_PLAYBACK) { id = pcm_data->dma_ch; - if (dai_id == LPASS_DP_RX) + if (dai_id == v->hdmi_dai) dmactl = drvdata->hdmi_rd_dmactl; else dmactl = drvdata->rd_dmactl; @@ -234,8 +234,7 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, return ret; } - switch (dai_id) { - case LPASS_DP_RX: + if (dai_id == v->hdmi_dai) { ret = regmap_fields_write(dmactl->burst8, id, LPAIF_DMACTL_BURSTEN_INCR4); if (ret) { @@ -254,9 +253,7 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, dev_err(soc_runtime->dev, "error updating dynbursten field: %d\n", ret); return ret; } - break; - case MI2S_PRIMARY: - case MI2S_SECONDARY: + } else { ret = regmap_fields_write(dmactl->intf, id, LPAIF_DMACTL_AUDINTF(dma_port)); if (ret) { @@ -264,12 +261,8 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, ret); return ret; } - - break; - default: - dev_err(soc_runtime->dev, "%s: invalid interface: %d\n", __func__, dai_id); - break; } + switch (bitwidth) { case 16: switch (channels) { @@ -299,22 +292,22 @@ static int lpass_platform_pcmops_hw_params(struct snd_soc_component *component, regval = LPAIF_DMACTL_WPSCNT_ONE; break; case 2: - regval = (dai_id == LPASS_DP_RX ? + regval = (dai_id == v->hdmi_dai ? LPAIF_DMACTL_WPSCNT_ONE : LPAIF_DMACTL_WPSCNT_TWO); break; case 4: - regval = (dai_id == LPASS_DP_RX ? + regval = (dai_id == v->hdmi_dai ? LPAIF_DMACTL_WPSCNT_TWO : LPAIF_DMACTL_WPSCNT_FOUR); break; case 6: - regval = (dai_id == LPASS_DP_RX ? + regval = (dai_id == v->hdmi_dai ? LPAIF_DMACTL_WPSCNT_THREE : LPAIF_DMACTL_WPSCNT_SIX); break; case 8: - regval = (dai_id == LPASS_DP_RX ? + regval = (dai_id == v->hdmi_dai ? LPAIF_DMACTL_WPSCNT_FOUR : LPAIF_DMACTL_WPSCNT_EIGHT); break; @@ -354,7 +347,7 @@ static int lpass_platform_pcmops_hw_free(struct snd_soc_component *component, struct regmap *map; unsigned int dai_id = cpu_dai->driver->id; - if (dai_id == LPASS_DP_RX) + if (dai_id == v->hdmi_dai) map = drvdata->hdmiif_map; else map = drvdata->lpaif_map; @@ -386,7 +379,7 @@ static int lpass_platform_pcmops_prepare(struct snd_soc_component *component, ch = pcm_data->dma_ch; if (dir == SNDRV_PCM_STREAM_PLAYBACK) { - if (dai_id == LPASS_DP_RX) { + if (dai_id == v->hdmi_dai) { dmactl = drvdata->hdmi_rd_dmactl; map = drvdata->hdmiif_map; } else { @@ -456,7 +449,7 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, ch = pcm_data->dma_ch; if (dir == SNDRV_PCM_STREAM_PLAYBACK) { id = pcm_data->dma_ch; - if (dai_id == LPASS_DP_RX) { + if (dai_id == v->hdmi_dai) { dmactl = drvdata->hdmi_rd_dmactl; map = drvdata->hdmiif_map; } else { @@ -480,8 +473,8 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, "error writing to rdmactl reg: %d\n", ret); return ret; } - switch (dai_id) { - case LPASS_DP_RX: + + if (dai_id == v->hdmi_dai) { ret = regmap_fields_write(dmactl->dyncclk, id, LPAIF_DMACTL_DYNCLK_ON); if (ret) { @@ -504,9 +497,7 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, LPAIF_IRQ_HDMI_REQ_ON_PRELOAD(ch) | LPAIF_IRQ_HDMI_METADONE | LPAIF_IRQ_HDMI_SDEEP_AUD_DIS(ch)); - break; - case MI2S_PRIMARY: - case MI2S_SECONDARY: + } else { reg_irqclr = LPAIF_IRQCLEAR_REG(v, LPAIF_IRQ_PORT_HOST); val_irqclr = LPAIF_IRQ_ALL(ch); @@ -514,10 +505,6 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, reg_irqen = LPAIF_IRQEN_REG(v, LPAIF_IRQ_PORT_HOST); val_mask = LPAIF_IRQ_ALL(ch); val_irqen = LPAIF_IRQ_ALL(ch); - break; - default: - dev_err(soc_runtime->dev, "%s: invalid %d interface\n", __func__, dai_id); - return -EINVAL; } ret = regmap_write(map, reg_irqclr, val_irqclr); @@ -541,8 +528,8 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, "error writing to rdmactl reg: %d\n", ret); return ret; } - switch (dai_id) { - case LPASS_DP_RX: + + if (dai_id == v->hdmi_dai) { ret = regmap_fields_write(dmactl->dyncclk, id, LPAIF_DMACTL_DYNCLK_OFF); if (ret) { @@ -556,16 +543,10 @@ static int lpass_platform_pcmops_trigger(struct snd_soc_component *component, LPAIF_IRQ_HDMI_METADONE | LPAIF_IRQ_HDMI_SDEEP_AUD_DIS(ch)); val_irqen = 0; - break; - case MI2S_PRIMARY: - case MI2S_SECONDARY: + } else { reg_irqen = LPAIF_IRQEN_REG(v, LPAIF_IRQ_PORT_HOST); val_mask = LPAIF_IRQ_ALL(ch); val_irqen = 0; - break; - default: - dev_err(soc_runtime->dev, "%s: invalid %d interface\n", __func__, dai_id); - return -EINVAL; } ret = regmap_update_bits(map, reg_irqen, val_mask, val_irqen); @@ -595,7 +576,7 @@ static snd_pcm_uframes_t lpass_platform_pcmops_pointer( struct regmap *map; unsigned int dai_id = cpu_dai->driver->id; - if (dai_id == LPASS_DP_RX) + if (dai_id == v->hdmi_dai) map = drvdata->hdmiif_map; else map = drvdata->lpaif_map; @@ -645,24 +626,18 @@ static irqreturn_t lpass_dma_interrupt_handler( struct regmap *map; unsigned int dai_id = cpu_dai->driver->id; - switch (dai_id) { - case LPASS_DP_RX: + if (dai_id == v->hdmi_dai) { map = drvdata->hdmiif_map; reg = LPASS_HDMITX_APP_IRQCLEAR_REG(v); val = (LPAIF_IRQ_HDMI_REQ_ON_PRELOAD(chan) | LPAIF_IRQ_HDMI_METADONE | LPAIF_IRQ_HDMI_SDEEP_AUD_DIS(chan)); - break; - case MI2S_PRIMARY: - case MI2S_SECONDARY: + } else { map = drvdata->lpaif_map; reg = LPAIF_IRQCLEAR_REG(v, LPAIF_IRQ_PORT_HOST); val = 0; - break; - default: - dev_err(soc_runtime->dev, "%s: invalid %d interface\n", __func__, dai_id); - return -EINVAL; } + if (interrupts & LPAIF_IRQ_PER(chan)) { rv = regmap_write(map, reg, LPAIF_IRQ_PER(chan) | val); @@ -826,10 +801,11 @@ static void lpass_platform_pcm_free(struct snd_soc_component *component, static int lpass_platform_pcmops_suspend(struct snd_soc_component *component) { struct lpass_data *drvdata = snd_soc_component_get_drvdata(component); + struct lpass_variant *v = drvdata->variant; struct regmap *map; unsigned int dai_id = component->id; - if (dai_id == LPASS_DP_RX) + if (dai_id == v->hdmi_dai) map = drvdata->hdmiif_map; else map = drvdata->lpaif_map; @@ -843,10 +819,11 @@ static int lpass_platform_pcmops_suspend(struct snd_soc_component *component) static int lpass_platform_pcmops_resume(struct snd_soc_component *component) { struct lpass_data *drvdata = snd_soc_component_get_drvdata(component); + struct lpass_variant *v = drvdata->variant; struct regmap *map; unsigned int dai_id = component->id; - if (dai_id == LPASS_DP_RX) + if (dai_id == v->hdmi_dai) map = drvdata->hdmiif_map; else map = drvdata->lpaif_map; diff --git a/sound/soc/qcom/lpass-sc7180.c b/sound/soc/qcom/lpass-sc7180.c index 85db650c2169..76bff06543ea 100644 --- a/sound/soc/qcom/lpass-sc7180.c +++ b/sound/soc/qcom/lpass-sc7180.c @@ -271,6 +271,7 @@ static struct lpass_variant sc7180_data = { .num_clks = 3, .dai_driver = sc7180_lpass_cpu_dai_driver, .num_dai = ARRAY_SIZE(sc7180_lpass_cpu_dai_driver), + .hdmi_dai = LPASS_DP_RX, .dai_osr_clk_names = (const char *[]) { "mclk0", "null", diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h index 0195372905ed..2de0632c1fb7 100644 --- a/sound/soc/qcom/lpass.h +++ b/sound/soc/qcom/lpass.h @@ -12,7 +12,6 @@ #include <linux/compiler.h> #include <linux/platform_device.h> #include <linux/regmap.h> -#include <dt-bindings/sound/sc7180-lpass.h> #include "lpass-hdmi.h" #define LPASS_AHBIX_CLOCK_FREQUENCY 131072000 @@ -20,6 +19,8 @@ #define LPASS_MAX_DMA_CHANNELS (8) #define LPASS_MAX_HDMI_DMA_CHANNELS (4) +#define LPASS_HDMI_DAI_UNSUPPORTED UINT_MAX + #define QCOM_REGMAP_FIELD_ALLOC(d, m, f, mf) \ do { \ mf = devm_regmap_field_alloc(d, m, f); \ @@ -245,6 +246,7 @@ struct lpass_variant { /* SOC specific dais */ struct snd_soc_dai_driver *dai_driver; int num_dai; + unsigned int hdmi_dai; const char * const *dai_osr_clk_names; const char * const *dai_bit_clk_names; -- 2.30.0

2 5

[PATCH v4 0/6] Add support for Rpi4b + Cirrus Lochnagar2 and CS47L15
by Richard Fitzgerald 15 Jan '21

15 Jan '21

This set of patches provides support for using the Raspberry Pi 4b with a Cirrus Logic Lochnagar 2 audio development platform plus Cirrus Logic CS47L15 codec. Patches are needed to audio-graph-card to enable support for setting the component sysclks and plls. These are not specific to CS47L15 - several Cirrus and non-Cirrus codecs also require component clock setup, typically for larger codecs with multiple internal clocking options and clock domains. The codec sysclks and plls cannot be placed under the clock framework because they are typically I2C/SPI/Soundwire-connected peripherals and access to the registers would cause a nested get of the bus clock. The clock framework does not support this and it would result in a deadlock. Richard Fitzgerald (6): of: base: Add of_count_phandle_with_fixed_args() dt-bindings: audio-graph-card: Add plls and sysclks properties ASoC: audio-graph-card: Support setting component plls and sysclks ASoC: madera: Allow codecs to be selected from kernel config ASoC: madera: Export clock config defines to dt-bindings ARM: dts: Add dts for RPi4b + Cirrus Logic Lochnagar2 + CS47L15 .../bindings/sound/audio-graph.yaml | 46 ++++ MAINTAINERS | 1 + arch/arm/boot/dts/Makefile | 1 + .../dts/bcm2711-rpi-4-b-lochnagar-cs47l15.dts | 186 ++++++++++++++ .../boot/dts/bcm2711-rpi-4-b-lochnagar.dtsi | 201 +++++++++++++++ drivers/of/base.c | 73 ++++-- include/dt-bindings/sound/madera.h | 60 +++++ include/linux/of.h | 9 + include/sound/simple_card_utils.h | 25 ++ sound/soc/codecs/Kconfig | 10 +- sound/soc/codecs/madera.h | 56 +---- sound/soc/generic/audio-graph-card.c | 13 + sound/soc/generic/simple-card-utils.c | 238 ++++++++++++++++++ 13 files changed, 836 insertions(+), 83 deletions(-) create mode 100644 arch/arm/boot/dts/bcm2711-rpi-4-b-lochnagar-cs47l15.dts create mode 100644 arch/arm/boot/dts/bcm2711-rpi-4-b-lochnagar.dtsi -- 2.20.1

4 21

[PATCH 1/2] MAINTAINERS: update maintainers of qcom audio
by Srinivas Kandagatla 15 Jan '21

15 Jan '21

Add myself as maintainer of qcom audio drivers, as Patrick has very little time to look at the patches. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla(a)linaro.org> Reviewed-by: Banajit Goswami <bgoswami(a)codeaurora.org> Acked-by: Patrick Lai <plai(a)codeaurora.org> --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 6eff4f720c72..b1545817c899 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14512,7 +14512,7 @@ S: Supported F: drivers/crypto/qat/ QCOM AUDIO (ASoC) DRIVERS -M: Patrick Lai <plai(a)codeaurora.org> +M: Srinivas Kandagatla <srinivas.kandagatla(a)linaro.org> M: Banajit Goswami <bgoswami(a)codeaurora.org> L: alsa-devel(a)alsa-project.org (moderated for non-subscribers) S: Supported -- 2.21.0

2 2

[PATCH 0/4] ASoC: qcom: Minor code cleanups for lpass-cpu
by Stephen Boyd 15 Jan '21

15 Jan '21

Here's some minor code cleanups for the lpass-cpu driver. I noticed that it casts away const from the driver data from DT. That's not great but fixing it is a little more involved. I'll get to it later. There's also some questionable clk_get() usage that should probably be clk_get_optional(). For now this should help a little. Cc: V Sujith Kumar Reddy <vsujithk(a)codeaurora.org> Cc: Srinivasa Rao <srivasam(a)codeaurora.org> Cc: Srinivas Kandagatla <srinivas.kandagatla(a)linaro.org> Cc: Cheng-Yi Chiang <cychiang(a)chromium.org> Stephen Boyd (4): ASoC: qcom: Remove useless debug print ASoC: qcom: Add some names to regmap configs ASoC: qcom: Stop casting away __iomem for error pointers ASoC: qcom: Remove duplicate error messages on ioremap sound/soc/qcom/lpass-cpu.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) base-commit: 5c8fe583cce542aa0b84adc939ce85293de36e5e -- https://chromeos.dev

3 6

Re: [PATCH 2/2] ASoC: mediatek: mt6359: add MT6359 accdet driver
by Mark Brown 15 Jan '21

15 Jan '21

On Wed, Jan 06, 2021 at 08:19:06PM +0800, Argus Lin wrote: > MT6359 audio codec support accessory detect features, adds MT6359 > accdet driver to support plug detection and key detection. > --- > sound/soc/codecs/Kconfig | 7 + > sound/soc/codecs/Makefile | 2 + > sound/soc/codecs/mt6359-accdet.c | 1951 ++++++++++++++++++++++++++++++++++++++ > sound/soc/codecs/mt6359-accdet.h | 136 +++ > sound/soc/codecs/mt6359.h | 1863 +++++++++++++++++++++++++++++++++--- This driver is *huge*. Looking through the code it feels like there's a lot of things that are written with mostly duplicated code that differs only in data so you could shrink things down a lot by refactoring things to have one copy of the code and pass different data into it. > Enable support for the platform which uses MT6359 as > external codec device. > +config SND_SOC_MT6359_ACCDET Missing blank line here. > +++ b/sound/soc/codecs/mt6359-accdet.c > @@ -0,0 +1,1951 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2021 MediaTek Inc. Please make the entire comment a C++ one so things look more intentional. > +#include "mt6359-accdet.h" > +#include "mt6359.h" > +/* grobal variable definitions */ Spelling mistake and you need more blank lines here. > +#define REGISTER_VAL(x) ((x) - 1) > +#define HAS_CAP(_c, _x) \ > + ({typeof(_c)c = (_c); \ > + typeof(_x)x = (_x); \ > + (((c) & (x)) == (x)); }) These need namepsacing. > +static struct mt63xx_accdet_data *accdet; > + > +static struct head_dts_data accdet_dts; > +struct pwm_deb_settings *cust_pwm_deb; You'd need a *very* good reason to be using global data rather than storing anything in the device's driver data like most drivers. There's extensive use of global data here, and lots of raw pr_ prints rather than dev_ prints as well - this doesn't look like how a Linux driver is supposed to be written. > + > +const struct of_device_id mt6359_accdet_of_match[] = { > + { > + .compatible = "mediatek,mt6359-accdet", > + .data = &mt6359_accdet, Given that this is specific to a particular PMIC why does this need a compatible string? > +/* global function declaration */ > + > +static u64 mt6359_accdet_get_current_time(void) > +{ > + return sched_clock(); > +} It is probably best to remove this wrapper. > +static bool mt6359_accdet_timeout_ns(u64 start_time_ns, u64 timeout_time_ns) > +{ > + u64 cur_time = 0; > + u64 elapse_time = 0; > + > + /* get current tick, ns */ > + cur_time = mt6359_accdet_get_current_time(); > + if (cur_time < start_time_ns) { > + start_time_ns = cur_time; > + /* 400us */ > + timeout_time_ns = 400 * 1000; > + } > + elapse_time = cur_time - start_time_ns; > + > + /* check if timeout */ > + if (timeout_time_ns <= elapse_time) > + return false; > + > + return true; > +} There must be a generic implementation of this already surely? > +static unsigned int check_key(unsigned int v) > +{ This looks a lot like open coding of the functionality of the extcon adc_jack functionality. > +static void send_key_event(unsigned int keycode, unsigned int flag) > +{ > + int report = 0; > + > + switch (keycode) { > + case DW_KEY: > + if (flag != 0) > + report = SND_JACK_BTN_1; What does flag mean? At the very least it needs renaming. > +static void send_status_event(unsigned int cable_type, unsigned int status) > +{ > + int report = 0; This is one of those places that looks like it could be code with different data passed in. > + > + switch (cable_type) { > + case HEADSET_NO_MIC: > + if (status) > + report = SND_JACK_HEADPHONE; > + else > + report = 0; > + snd_soc_jack_report(&accdet->jack, report, SND_JACK_HEADPHONE); > + /* when plug 4-pole out, if both AB=3 AB=0 happen,3-pole plug > + * in will be incorrectly reported, then 3-pole plug-out is > + * reported,if no mantory 4-pole plug-out, icon would be > + * visible. > + */ > + if (status == 0) { > + report = 0; > + snd_soc_jack_report(&accdet->jack, report, SND_JACK_MICROPHONE); > + } > + pr_info("accdet HEADPHONE(3-pole) %s\n", > + status ? "PlugIn" : "PlugOut"); You shouldn't be spamming the logs for normal events like this. > + regmap_read(accdet->regmap, ACCDET_IRQ_ADDR, &val); > + while (val & ACCDET_IRQ_MASK_SFT && > + mt6359_accdet_timeout_ns(cur_time, ACCDET_TIME_OUT)) > + ; This is open coding regmap_read_poll_timeout(), this pattern is repeated in several places. > +static inline void clear_accdet_eint(unsigned int eintid) > +{ > + if ((eintid & PMIC_EINT0) == PMIC_EINT0) { The == part is redundant here, and again this is another place where it feels like there's duplicated code that should be using data. All this interrupt handling code is really extremely difficult to follow, there's *lots* of functions all open coding many individual register bits sometimes redundantly and it's very hard to follow what it's supposed to be doing. I can't help but think that in addition to making things data driven writing more linear code without these abstraction layers would help a lot with comprehensibility. > +static irqreturn_t mtk_accdet_irq_handler_thread(int irq, void *data) > +{ > + accdet_irq_handle(); > + > + return IRQ_HANDLED; > +} Why does this wrapper function exist - AFAICT it's just introducing a bug given that the called function is able to detect spurious interrupts but this unconditionally reports IRQ_HANDLED. > +int mt6359_accdet_init(struct snd_soc_component *component, > + struct snd_soc_card *card) > +{ > + int ret = 0; > + struct mt63xx_accdet_data *priv = > + snd_soc_card_get_drvdata(component->card); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(mt6359_accdet_init); This is a weird interface, what's going on here? > +int mt6359_accdet_set_drvdata(struct snd_soc_card *card) > +{ > + snd_soc_card_set_drvdata(card, accdet); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(mt6359_accdet_set_drvdata); This is setting off *massive* alarm bells in that it seems to try to claim the card level driver data for this specific driver, again what's going on here? > +module_init(mt6359_accdet_soc_init); > +module_exit(mt6359_accdet_soc_exit); module_platform_driver()

1 0

[PATCH][next] ALSA: usb-audio: remove initialization of param_period_time_if_needed
by Colin King 15 Jan '21

15 Jan '21

From: Colin Ian King <colin.king(a)canonical.com> Variable param_period_time_if_needed is being initialized with a value that is never read, the assignment is redundant and can be removed. Addresses-Coverity: ("Unused value") Signed-off-by: Colin Ian King <colin.king(a)canonical.com> --- sound/usb/pcm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index f71965bf815f..ed654338f6b7 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -934,7 +934,7 @@ static int setup_hw_info(struct snd_pcm_runtime *runtime, struct snd_usb_substre { const struct audioformat *fp; unsigned int pt, ptmin; - int param_period_time_if_needed = -1; + int param_period_time_if_needed; int err; runtime->hw.formats = subs->formats; -- 2.29.2

2 1

[PATCH v2 0/5] Clock and reset improvements for Tegra ALSA drivers
by Dmitry Osipenko 15 Jan '21

15 Jan '21

This series improves the handling of clock and reset controls of NVIDA Tegra ALSA drivers. Tegra HDA and AHUB drivers aren't handling resets properly, which needs to be fixed in order to unblock other patches related to fixes of the reset controller driver since HDA/AHUB are bound to fail once reset controller driver will be corrected. In particular ALSA drivers are relying on implicit de-assertion of resets which is done by the tegra-clk driver. It's not the business of the clk driver to touch resets and we need to fix this because it breaks reset/clk programming sequences of other Tegra drivers. Changelog: v2: - Added regcache_sync() to the "ahub: Reset hardware properly" patch, which was missed by accident in v1. - Corrected typo in the format of the error message in "ahub: Use of_reset_control_array_get_exclusive()" patch by s/%p/%pe/. Dmitry Osipenko (5): ALSA: hda/tegra: Use clk_bulk helpers ALSA: hda/tegra: Reset hardware ASoC: tegra: ahub: Use of_reset_control_array_get_exclusive() ASoC: tegra: ahub: Use clk_bulk helpers ASoC: tegra: ahub: Reset hardware properly sound/pci/hda/hda_tegra.c | 86 ++++++++----------------- sound/soc/tegra/tegra30_ahub.c | 113 +++++++++------------------------ sound/soc/tegra/tegra30_ahub.h | 6 +- 3 files changed, 59 insertions(+), 146 deletions(-) -- 2.29.2

1 6

Sound stutters with TC-Helicon GoXLR
by GitHub issues - opened 15 Jan '21

15 Jan '21

alsa-project/alsa-lib issue #113 was opened from Toniob: Hi, I bought a hardware mixer from TC-Helicon. I mainly use it under windows for streaming. But now, I'm trying to make it work under Linux, which I use for work. The hardware is seen as a multi-inputs multi-outputs soundcard under Linux. I use Debian unstable, Gnome (xwayland) and pulseaudio. It's an usb connected hardware (with its own power). The sound was stuttering every 3 or 4 seconds. It was plugged on a usb 3.0 port on the motherboard. But then, I bought a pci-express usb 3.0 card and I plugged the goxlr on it. The stutters are steal present but less often (I'd say on every 30 seconds). I've opened an issue on pulseaudio gitlab but it doesn't seem related to pulse as I've got the issue with alsa only : https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/issues/1093 . I know a person, met on the discord of tc-helicon, who has exactly the same symptoms with another distribution. Another fun fact is that the inputs do not stutter. I can record my mic plugged in the goxlr and it never stutters. Issue URL : https://github.com/alsa-project/alsa-lib/issues/113 Repository URL: https://github.com/alsa-project/alsa-lib

1 0

[PATCH] ALSA: hda: Add Cometlake-R PCI ID
by Kai-Chuan Hsieh 15 Jan '21

15 Jan '21

Add HD Audio Device PCI ID for the Intel Cometlake-R platform Reviewed-by: Kai Vehmanen <kai.vehmanen(a)linux.intel.com> Signed-off-by: Kai-Chuan Hsieh <kaichuan.hsieh(a)canonical.com> --- sound/pci/hda/hda_intel.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 83a4a6290070..313d2c9fb35d 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2482,6 +2482,9 @@ static const struct pci_device_id azx_ids[] = { /* CometLake-S */ { PCI_DEVICE(0x8086, 0xa3f0), .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE}, + /* CometLake-R */ + { PCI_DEVICE(0x8086, 0xf0c8), + .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE}, /* Icelake */ { PCI_DEVICE(0x8086, 0x34c8), .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE}, -- 2.25.1

2 1

[PATCH] ALSA: dice: add support for Lexicon I-ONIX FW810s
by Takashi Sakamoto 15 Jan '21

15 Jan '21

I-ONIX FW810s was shipped in Lexicon brand of HARMAN International industries, Inc 2009. The model uses TCD2220 ASIC as its communication engine. TCAT general protocol is supported, its extension isn't. This patch adds support for the model with hard-coded stream formats. $ python3 ~/git/linux-firewire-utils/src/crpp < /sys/bus/firewire/devices/fw1/config_rom ROM header and bus information block ----------------------------------------------------------------- 400 04042b91 bus_info_length 4, crc_length 4, crc 11153 404 31333934 bus_name "1394" 408 e0008102 irmc 1, cmc 1, isc 1, bmc 0, cyc_clk_acc 0, max_rec 8 (512) 40c 000fd720 company_id 000fd7 | 410 007d7ecf device_id 20007d7ecf | EUI-64 000fd720007d7ecf root directory ----------------------------------------------------------------- 414 00064c2d directory_length 6, crc 19501 418 03000fd7 vendor 41c 8100000a --> descriptor leaf at 444 420 17000001 model 424 8100000d --> descriptor leaf at 458 428 0c0087c0 node capabilities per IEEE 1394 42c d1000001 --> unit directory at 430 unit directory at 430 ----------------------------------------------------------------- 430 000438f2 directory_length 4, crc 14578 434 12000fd7 specifier id 438 13000001 version 43c 17000001 model 440 8100000d --> descriptor leaf at 474 descriptor leaf at 444 ----------------------------------------------------------------- 444 000489d5 leaf_length 4, crc 35285 448 00000000 textual descriptor 44c 00000000 minimal ASCII 450 4c657869 "Lexi" 454 636f6e00 "con" descriptor leaf at 458 ----------------------------------------------------------------- 458 0006594b leaf_length 6, crc 22859 45c 00000000 textual descriptor 460 00000000 minimal ASCII 464 492d4f4e "I-ON" 468 49585f46 "IX_F" 46c 57383130 "W810" 470 53000000 "S" descriptor leaf at 474 ----------------------------------------------------------------- 474 0006594b leaf_length 6, crc 22859 478 00000000 textual descriptor 47c 00000000 minimal ASCII 480 492d4f4e "I-ON" 484 49585f46 "IX_F" 488 57383130 "W810" 48c 53000000 "S" Signed-off-by: Takashi Sakamoto <o-takashi(a)sakamocchi.jp> --- sound/firewire/dice/Makefile | 3 ++- sound/firewire/dice/dice-harman.c | 26 ++++++++++++++++++++++++++ sound/firewire/dice/dice.c | 12 ++++++++++++ sound/firewire/dice/dice.h | 1 + 4 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 sound/firewire/dice/dice-harman.c diff --git a/sound/firewire/dice/Makefile b/sound/firewire/dice/Makefile index 7a62dafd0f78..9bf7b960a720 100644 --- a/sound/firewire/dice/Makefile +++ b/sound/firewire/dice/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only snd-dice-objs := dice-transaction.o dice-stream.o dice-proc.o dice-midi.o \ dice-pcm.o dice-hwdep.o dice.o dice-tcelectronic.o \ - dice-alesis.o dice-extension.o dice-mytek.o dice-presonus.o + dice-alesis.o dice-extension.o dice-mytek.o dice-presonus.o \ + dice-harman.o obj-$(CONFIG_SND_DICE) += snd-dice.o diff --git a/sound/firewire/dice/dice-harman.c b/sound/firewire/dice/dice-harman.c new file mode 100644 index 000000000000..a8ca00c397e8 --- /dev/null +++ b/sound/firewire/dice/dice-harman.c @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: GPL-2.0 +// dice-harman.c - a part of driver for DICE based devices +// +// Copyright (c) 2021 Takashi Sakamoto +// +// Licensed under the terms of the GNU General Public License, version 2. + +#include "dice.h" + +int snd_dice_detect_harman_formats(struct snd_dice *dice) +{ + int i; + + // Lexicon I-ONYX FW810s supports sampling transfer frequency up to + // 96.0 kHz, 12 PCM channels and 1 MIDI channel in its first tx stream + // , 10 PCM channels and 1 MIDI channel in its first rx stream for all + // of the frequencies. + for (i = 0; i < 2; ++i) { + dice->tx_pcm_chs[0][i] = 12; + dice->tx_midi_ports[0] = 1; + dice->rx_pcm_chs[0][i] = 10; + dice->rx_midi_ports[0] = 1; + } + + return 0; +} diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c index 06c94f009dfb..107a81691f0e 100644 --- a/sound/firewire/dice/dice.c +++ b/sound/firewire/dice/dice.c @@ -20,10 +20,12 @@ MODULE_LICENSE("GPL v2"); #define OUI_MYTEK 0x001ee8 #define OUI_SSL 0x0050c2 // Actually ID reserved by IEEE. #define OUI_PRESONUS 0x000a92 +#define OUI_HARMAN 0x000fd7 #define DICE_CATEGORY_ID 0x04 #define WEISS_CATEGORY_ID 0x00 #define LOUD_CATEGORY_ID 0x10 +#define HARMAN_CATEGORY_ID 0x20 #define MODEL_ALESIS_IO_BOTH 0x000001 @@ -56,6 +58,8 @@ static int check_dice_category(struct fw_unit *unit) category = WEISS_CATEGORY_ID; else if (vendor == OUI_LOUD) category = LOUD_CATEGORY_ID; + else if (vendor == OUI_HARMAN) + category = HARMAN_CATEGORY_ID; else category = DICE_CATEGORY_ID; if (device->config_rom[3] != ((vendor << 8) | category) || @@ -388,6 +392,14 @@ static const struct ieee1394_device_id dice_id_table[] = { .model_id = 0x000008, .driver_data = (kernel_ulong_t)snd_dice_detect_presonus_formats, }, + // Lexicon I-ONYX FW810S. + { + .match_flags = IEEE1394_MATCH_VENDOR_ID | + IEEE1394_MATCH_MODEL_ID, + .vendor_id = OUI_HARMAN, + .model_id = 0x000001, + .driver_data = (kernel_ulong_t)snd_dice_detect_harman_formats, + }, { .match_flags = IEEE1394_MATCH_VERSION, .version = DICE_INTERFACE, diff --git a/sound/firewire/dice/dice.h b/sound/firewire/dice/dice.h index 7fbffcab94c2..adc6f7c84460 100644 --- a/sound/firewire/dice/dice.h +++ b/sound/firewire/dice/dice.h @@ -233,5 +233,6 @@ int snd_dice_detect_alesis_mastercontrol_formats(struct snd_dice *dice); int snd_dice_detect_extension_formats(struct snd_dice *dice); int snd_dice_detect_mytek_formats(struct snd_dice *dice); int snd_dice_detect_presonus_formats(struct snd_dice *dice); +int snd_dice_detect_harman_formats(struct snd_dice *dice); #endif -- 2.27.0

2 1