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

[Srinivasa Rao Mandadapu <srivasam@codeaurora.org>]

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@codeaurora.org>
>>
>> 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@codeaurora.org>
>> Signed-off-by: V Sujith Kumar Reddy <vsujithk@codeaurora.org>
>> ---
>>   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.