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;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <1598855964-1042-4-git-send-email-srivasam@codeaurora.org>
References: <1598855964-1042-1-git-send-email-srivasam@codeaurora.org> <1598855964-1042-4-git-send-email-srivasam@codeaurora.org>
Subject: Re: [PATCH v3 3/5] ASoC: qcom: Add support for lpass hdmi driver
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     V Sujith Kumar Reddy <vsujithk@codeaurora.org>,
        Srinivasa Rao <srivasam@codeaurora.org>
To:     Srinivasa Rao Mandadapu <srivasam@codeaurora.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
Date:   Tue, 01 Sep 2020 00:36:34 -0700
Message-ID: <159894579447.334488.1604209140945740724@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9.1
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 *substre=
am,
> +               struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
> +{
[...]
> +       data_format =3D LPASS_DATA_FORMAT_LINEAR;
> +       ch_sts_buf0 =3D (((data_format << LPASS_DATA_FORMAT_SHIFT) & LPAS=
S_DATA_FORMAT_MASK)
> +                               | ((sampling_freq << LPASS_FREQ_BIT_SHIFT=
) & LPASS_FREQ_BIT_MASK));
> +       ch_sts_buf1 =3D (word_length) & LPASS_WORDLENGTH_MASK;
> +
> +       ret =3D 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?

> +                                       __func__, ret);
> +               return ret;
> +       }
> +
> +       ret =3D 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 =3D 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, BA=
SE)
> diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platf=
orm.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)
> =20
>         /* Handle per channel interrupts */
>         for (chan =3D 0; chan < LPASS_MAX_DMA_CHANNELS; chan++) {
> -               if (irqs & LPAIF_IRQ_ALL(chan) && drvdata->substream[chan=
]) {
> +               switch (v->id) {
> +               case HDMI_INTERFACE:
> +                       val =3D LPAIF_IRQ_HDMI_REQ_ON_PRELOAD(chan) |
> +                               LPAIF_IRQ_HDMI_METADONE |
> +                               LPAIF_IRQ_HDMI_SDEEP_AUD_DIS(chan);
> +                       break;
> +               case I2S_INTERFACE:
> +                       val =3D 0;
> +                       break;
> +               default:
> +                       pr_err("%s: invalid  %d interface\n", __func__, v=
->id);

Any reason we can't use dev_err() here?

> +                       return -EINVAL;
> +               }
> +               if (irqs & (LPAIF_IRQ_ALL(chan) | val)
> +                       && drvdata->substream[chan]) {
>                         rv =3D lpass_dma_interrupt_handler(
>                                                 drvdata->substream[chan],
>                                                 drvdata, chan, irqs);
> @@ -644,15 +807,15 @@ int asoc_qcom_lpass_platform_register(struct platfo=
rm_device *pdev)
> =20
>         /* ensure audio hardware is disabled */
>         ret =3D 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", r=
et);
>                 return ret;
>         }
> =20
>         ret =3D 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?

> +               pdev->name, drvdata);
>         if (ret) {
>                 dev_err(&pdev->dev, "irq request failed: %d\n", ret);
>                 return ret;