Messages in this thread | | | Subject | Re: [PATCH v3 3/5] ASoC: qcom: Add support for lpass hdmi driver | From | Srinivasa Rao Mandadapu <> | Date | Thu, 3 Sep 2020 14:59:41 +0530 |
| |
Thanks Stephen for your time for review!!!
On 9/1/2020 1:06 PM, Stephen Boyd wrote: > Quoting Srinivasa Rao Mandadapu (2020-08-30 23:39:22) >> diff --git a/sound/soc/qcom/lpass-hdmi.c b/sound/soc/qcom/lpass-hdmi.c >> new file mode 100644 >> index 0000000..7e18113 >> --- /dev/null >> +++ b/sound/soc/qcom/lpass-hdmi.c >> @@ -0,0 +1,684 @@ >> +// SPDX-License-Identifier: GPL-2.0-only > [...] >> + >> +static int lpass_hdmi_daiops_hw_params(struct snd_pcm_substream *substream, >> + struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) >> +{ > [...] >> + data_format = LPASS_DATA_FORMAT_LINEAR; >> + ch_sts_buf0 = (((data_format << LPASS_DATA_FORMAT_SHIFT) & LPASS_DATA_FORMAT_MASK) >> + | ((sampling_freq << LPASS_FREQ_BIT_SHIFT) & LPASS_FREQ_BIT_MASK)); >> + ch_sts_buf1 = (word_length) & LPASS_WORDLENGTH_MASK; >> + >> + ret = regmap_field_write(drvdata->tx_ctl->soft_reset, LPASS_TX_CTL_RESET); >> + if (ret) { >> + dev_err(dai->dev, "%s error writing to softreset enable : %d\n", > All of these strings bloat the kernel image. Can we just return ret > instead and if something goes wrong we can use a debug patch to figure > out which register write failed? Would a register write even fail to > begin with? Will remove wherever possible. > >> + __func__, ret); >> + return ret; >> + } >> + >> + ret = regmap_field_write(drvdata->tx_ctl->soft_reset, LPASS_TX_CTL_CLEAR); >> + if (ret) { >> + dev_err(dai->dev, "%s error writing to softreset disable : %d\n", >> + __func__, ret); >> + return ret; >> + } >> + >> + ret = regmap_field_write(drvdata->legacy->legacy_en, >> + LPASS_HDMITX_LEGACY_DISABLE); >> + if (ret) { >> + dev_err(dai->dev, "%s error writing to legacy_en field : %d\n", >> + __func__, ret); >> + return ret; >> + } >> + > [...] >> #define LPAIF_DMACTL_REG(v, chan, dir) __LPAIF_DMA_REG(v, chan, dir, CTL) >> #define LPAIF_DMABASE_REG(v, chan, dir) __LPAIF_DMA_REG(v, chan, dir, BASE) >> diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c >> index df692ed..607f4c4 100644 >> --- a/sound/soc/qcom/lpass-platform.c >> +++ b/sound/soc/qcom/lpass-platform.c >> @@ -553,7 +702,21 @@ static irqreturn_t lpass_platform_lpaif_irq(int irq, void *data) >> >> /* Handle per channel interrupts */ >> for (chan = 0; chan < LPASS_MAX_DMA_CHANNELS; chan++) { >> - if (irqs & LPAIF_IRQ_ALL(chan) && drvdata->substream[chan]) { >> + switch (v->id) { >> + case HDMI_INTERFACE: >> + val = LPAIF_IRQ_HDMI_REQ_ON_PRELOAD(chan) | >> + LPAIF_IRQ_HDMI_METADONE | >> + LPAIF_IRQ_HDMI_SDEEP_AUD_DIS(chan); >> + break; >> + case I2S_INTERFACE: >> + val = 0; >> + break; >> + default: >> + pr_err("%s: invalid %d interface\n", __func__, v->id); > Any reason we can't use dev_err() here?
It's interrupt handler. Here dev variable is not available.
So we shall remove print as per your previous comment or keep as it is.
> >> + return -EINVAL; >> + } >> + if (irqs & (LPAIF_IRQ_ALL(chan) | val) >> + && drvdata->substream[chan]) { >> rv = lpass_dma_interrupt_handler( >> drvdata->substream[chan], >> drvdata, chan, irqs); >> @@ -644,15 +807,15 @@ int asoc_qcom_lpass_platform_register(struct platform_device *pdev) >> >> /* ensure audio hardware is disabled */ >> ret = regmap_write(drvdata->lpaif_map, >> - LPAIF_IRQEN_REG(v, LPAIF_IRQ_PORT_HOST), 0); >> + IRQ_EN(v, LPAIF_IRQ_PORT_HOST), 0); >> if (ret) { >> dev_err(&pdev->dev, "error writing to irqen reg: %d\n", ret); >> return ret; >> } >> >> ret = devm_request_irq(&pdev->dev, drvdata->lpaif_irq, >> - lpass_platform_lpaif_irq, IRQF_TRIGGER_RISING, >> - "lpass-irq-lpaif", drvdata); >> + lpass_platform_lpaif_irq, IRQF_TRIGGER_RISING, > Can we use the irq flags from the firmware, i.e. whatever the DT or ACPI > tables say? okay. > >> + pdev->name, drvdata); >> if (ret) { >> dev_err(&pdev->dev, "irq request failed: %d\n", ret); >> return ret;
-- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
| |