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

[Srinivasa Rao Mandadapu]

Thanks Srinivas For Review!!!

On 9/4/2020 4:11 PM, Srinivas Kandagatla wrote:
> On 31/08/2020 07:39, Srinivasa Rao Mandadapu wrote:
> > From: V Sujith Kumar Reddy <vsujithk@xxxxxxxxxxxxxx>
> >
> > Upadate lpass cpu and platform driver to support audio over dp.
> > Also add lpass-hdmi.c and lpass-hdmi.h.
> >
> > Signed-off-by: Srinivasa Rao <srivasam@xxxxxxxxxxxxxx>
> > Signed-off-by: V Sujith Kumar Reddy <vsujithk@xxxxxxxxxxxxxx>
> > ---
> >   sound/soc/qcom/Kconfig           |   5 +
> >   sound/soc/qcom/Makefile          |   2 +
> >   sound/soc/qcom/lpass-apq8016.c   |   1 +
> >   sound/soc/qcom/lpass-cpu.c       |  64 ++--
> >   sound/soc/qcom/lpass-hdmi.c      | 684 
> > +++++++++++++++++++++++++++++++++++++++
> >   sound/soc/qcom/lpass-hdmi.h      | 129 ++++++++
> >   sound/soc/qcom/lpass-ipq806x.c   |   1 +
> >   sound/soc/qcom/lpass-lpaif-reg.h |  48 ++-
> >   sound/soc/qcom/lpass-platform.c  | 287 ++++++++++++----
> >   sound/soc/qcom/lpass.h           |  88 ++++-
> >   10 files changed, 1225 insertions(+), 84 deletions(-)
> >   create mode 100644 sound/soc/qcom/lpass-hdmi.c
> >   create mode 100644 sound/soc/qcom/lpass-hdmi.h
> >
> > diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
> > index a607ace..509584c 100644
> > --- a/sound/soc/qcom/Kconfig
> > +++ b/sound/soc/qcom/Kconfig
> > @@ -12,6 +12,10 @@ config SND_SOC_LPASS_CPU
> >       tristate
> >       select REGMAP_MMIO
> >   +config SND_SOC_LPASS_HDMI
> > +    tristate
> > +    select REGMAP_MMIO
> > +
> >   config SND_SOC_LPASS_PLATFORM
> >       tristate
> >       select REGMAP_MMIO
> > @@ -30,6 +34,7 @@ config SND_SOC_LPASS_SC7180
> >       tristate
> >       select SND_SOC_LPASS_CPU
> >       select SND_SOC_LPASS_PLATFORM
> > +    select SND_SOC_LPASS_HDMI
> >     config SND_SOC_STORM
> >       tristate "ASoC I2S support for Storm boards"
> > diff --git a/sound/soc/qcom/Makefile b/sound/soc/qcom/Makefile
> > index 7972c94..0bd90d7 100644
> > --- a/sound/soc/qcom/Makefile
> > +++ b/sound/soc/qcom/Makefile
> > @@ -1,12 +1,14 @@
> >   # SPDX-License-Identifier: GPL-2.0
> >   # Platform
> >   snd-soc-lpass-cpu-objs := lpass-cpu.o
> > +snd-soc-lpass-hdmi-objs := lpass-hdmi.o
> >   snd-soc-lpass-platform-objs := lpass-platform.o
> >   snd-soc-lpass-ipq806x-objs := lpass-ipq806x.o
> >   snd-soc-lpass-apq8016-objs := lpass-apq8016.o
> >   snd-soc-lpass-sc7180-objs := lpass-sc7180.o
> >     obj-$(CONFIG_SND_SOC_LPASS_CPU) += snd-soc-lpass-cpu.o
> > +obj-$(CONFIG_SND_SOC_LPASS_HDMI) += snd-soc-lpass-hdmi.o
> >   obj-$(CONFIG_SND_SOC_LPASS_PLATFORM) += snd-soc-lpass-platform.o
> >   obj-$(CONFIG_SND_SOC_LPASS_IPQ806X) += snd-soc-lpass-ipq806x.o
> >   obj-$(CONFIG_SND_SOC_LPASS_APQ8016) += snd-soc-lpass-apq8016.o
> > diff --git a/sound/soc/qcom/lpass-apq8016.c 
> > b/sound/soc/qcom/lpass-apq8016.c
> > index 5c8ae22..a1bc7e2 100644
> > --- a/sound/soc/qcom/lpass-apq8016.c
> > +++ b/sound/soc/qcom/lpass-apq8016.c
> > @@ -289,6 +289,7 @@ static struct lpass_variant apq8016_data = {
> >       .exit            = apq8016_lpass_exit,
> >       .alloc_dma_channel    = apq8016_lpass_alloc_dma_channel,
> >       .free_dma_channel    = apq8016_lpass_free_dma_channel,
> > +    .id            = I2S_INTERFACE,
>
> Before going into detail review, I see real issue in the overall 
> approach here to add new interface to exiting lpass!!
>
> Intention of struct lpass_variant is to address differences between 
> SoCs or different lpass versions. But you should not duplicate this 
> and use it for addressing differences between each lpass interfaces!
> All the dai related register offsets should still go in to this 
> structure and driver should be able to know which dai its talking to 
> based on snd_soc_dai_driver id and select correct register offset.
Do You suggest to use separate structure like struct 
lpass_hdmi_interface in lpass_data?
> Also on the other note, can you please split the patch if possible so 
> that it will be easy for review. Specially I would like to see header 
> file changes specific to adding new interface to be separate then 
> followed by the actual interface implementation  and then the user.
Okay, will split the patch.
> I also see some unrelated changes like changing buffer sizes, which 
> should go into different patch!
Okay we will separate the patch.
> --srini

--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.