Re: [PATCH v3 3/5] ASoC: qcom: Add support for lpass hdmi driver

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;