Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 06E20C433C9
Subject: Re: [PATCH v3 3/5] ASoC: qcom: Add support for lpass hdmi driver
To:     Stephen Boyd <swboyd@chromium.org>, agross@kernel.org,
        alsa-devel@alsa-project.org, bgoswami@codeaurora.org,
        bjorn.andersson@linaro.org, broonie@kernel.org,
        devicetree@vger.kernel.org, lgirdwood@gmail.com,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        perex@perex.cz, plai@codeaurora.org, robh+dt@kernel.org,
        rohitkr@codeaurora.org, srinivas.kandagatla@linaro.org,
        tiwai@suse.com
Cc:     V Sujith Kumar Reddy <vsujithk@codeaurora.org>
References: <1598855964-1042-1-git-send-email-srivasam@codeaurora.org>
 <1598855964-1042-4-git-send-email-srivasam@codeaurora.org>
 <159894579447.334488.1604209140945740724@swboyd.mtv.corp.google.com>
From:   Srinivasa Rao Mandadapu <srivasam@codeaurora.org>
Organization: Qualcomm India Private Limited
Message-ID: <36672d15-78bf-ffe7-71a3-4a7ed8664149@codeaurora.org>
Date:   Thu, 3 Sep 2020 14:59:41 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.2.1
MIME-Version: 1.0
In-Reply-To: <159894579447.334488.1604209140945740724@swboyd.mtv.corp.google.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.