2021-06-09 13:55:15

by Srinivas Kandagatla

[permalink] [raw]
Subject: Re: [PATCH v2] ASoC: qcom: Add four speaker support on MI2S secondary



On 09/06/2021 08:13, Srinivasa Rao Mandadapu wrote:
> Add four speaker support on MI2S secondary block
> by using I2S SD1 line on gpio52 pin, and add channel map
> control support in the lpass-cpu audio driver.
>
> Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
> ---
> Changes Since V1:
> -- removed set_channel_map/get_channel_map implementation as default kcontrols
> added in pcm_new API.
>
> sound/soc/qcom/lpass-cpu.c | 32 ++++++++++++++++++++++++++++++++
> sound/soc/qcom/lpass-sc7180.c | 1 +
> sound/soc/qcom/lpass.h | 2 ++
> 3 files changed, 35 insertions(+)
>
> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> index c62d2612e8f5..15d69e311b38 100644
> --- a/sound/soc/qcom/lpass-cpu.c
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -29,6 +29,14 @@
> #define LPASS_CPU_I2S_SD0_1_2_MASK GENMASK(2, 0)
> #define LPASS_CPU_I2S_SD0_1_2_3_MASK GENMASK(3, 0)
>
> +/*
> + * Channel maps for Quad channel playbacks on MI2S Secondary
> + */
> +static struct snd_pcm_chmap_elem lpass_quad_chmaps = {
> + .channels = 4,
> + .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_RL,
> + SNDRV_CHMAP_FR, SNDRV_CHMAP_RR }

AFAIU, You would need an empty entry at the end of this list if not we
will endup in a dereferencing memory sitting right next to this array
which will lead to random memory corruptions...

Have a look at some of the examples in existing codec drivers.

Other than that patch looks good.

--srini


> +};
> static int lpass_cpu_init_i2sctl_bitfields(struct device *dev,
> struct lpaif_i2sctl *i2sctl, struct regmap *map)
> {
> @@ -324,6 +332,25 @@ const struct snd_soc_dai_ops asoc_qcom_lpass_cpu_dai_ops = {
> };
> EXPORT_SYMBOL_GPL(asoc_qcom_lpass_cpu_dai_ops);
>
> +int lpass_cpu_pcm_new(struct snd_soc_pcm_runtime *rtd,
> + struct snd_soc_dai *dai)
> +{
> + int ret;
> + struct snd_soc_dai_driver *drv = dai->driver;
> + struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
> +
> + if (drvdata->mi2s_playback_sd_mode[dai->id] == LPAIF_I2SCTL_MODE_QUAD01) {
> + ret = snd_pcm_add_chmap_ctls(rtd->pcm, SNDRV_PCM_STREAM_PLAYBACK,
> + &lpass_quad_chmaps, drv->playback.channels_max, 0,
> + NULL);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(lpass_cpu_pcm_new);
> +
> int asoc_qcom_lpass_cpu_dai_probe(struct snd_soc_dai *dai)
> {
> struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
> @@ -856,6 +883,11 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
> PTR_ERR(drvdata->mi2s_bit_clk[dai_id]));
> return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]);
> }
> + if (drvdata->mi2s_playback_sd_mode[dai_id] ==
> + LPAIF_I2SCTL_MODE_QUAD01) {
> + variant->dai_driver[dai_id].playback.channels_min = 4;
> + variant->dai_driver[dai_id].playback.channels_max = 4;
> + }
> }
>
> /* Allocation for i2sctl regmap fields */
> diff --git a/sound/soc/qcom/lpass-sc7180.c b/sound/soc/qcom/lpass-sc7180.c
> index 8c168d3c589e..77a556b27cf0 100644
> --- a/sound/soc/qcom/lpass-sc7180.c
> +++ b/sound/soc/qcom/lpass-sc7180.c
> @@ -58,6 +58,7 @@ static struct snd_soc_dai_driver sc7180_lpass_cpu_dai_driver[] = {
> },
> .probe = &asoc_qcom_lpass_cpu_dai_probe,
> .ops = &asoc_qcom_lpass_cpu_dai_ops,
> + .pcm_new = lpass_cpu_pcm_new,
> }, {
> .id = LPASS_DP_RX,
> .name = "Hdmi",
> diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
> index 83b2e08ade06..623ddccdafff 100644
> --- a/sound/soc/qcom/lpass.h
> +++ b/sound/soc/qcom/lpass.h
> @@ -259,5 +259,7 @@ void asoc_qcom_lpass_cpu_platform_shutdown(struct platform_device *pdev);
> int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev);
> int asoc_qcom_lpass_cpu_dai_probe(struct snd_soc_dai *dai);
> extern const struct snd_soc_dai_ops asoc_qcom_lpass_cpu_dai_ops;
> +int lpass_cpu_pcm_new(struct snd_soc_pcm_runtime *rtd,
> + struct snd_soc_dai *dai);
>
> #endif /* __LPASS_H__ */
>


2021-06-09 17:32:12

by Srinivasa Rao Mandadapu

[permalink] [raw]
Subject: Re: [PATCH v2] ASoC: qcom: Add four speaker support on MI2S secondary

Hi Srini,

Thanks for your Time!!!

On 6/9/2021 1:40 PM, Srinivas Kandagatla wrote:
>
>
> On 09/06/2021 08:13, Srinivasa Rao Mandadapu wrote:
>> Add four speaker support on MI2S secondary block
>> by using I2S SD1 line on gpio52 pin, and add channel map
>> control support in the lpass-cpu audio driver.
>>
>> Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
>> ---
>> Changes Since V1:
>>     -- removed set_channel_map/get_channel_map implementation as
>> default kcontrols
>>         added in pcm_new API.
>>
>>   sound/soc/qcom/lpass-cpu.c    | 32 ++++++++++++++++++++++++++++++++
>>   sound/soc/qcom/lpass-sc7180.c |  1 +
>>   sound/soc/qcom/lpass.h        |  2 ++
>>   3 files changed, 35 insertions(+)
>>
>> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
>> index c62d2612e8f5..15d69e311b38 100644
>> --- a/sound/soc/qcom/lpass-cpu.c
>> +++ b/sound/soc/qcom/lpass-cpu.c
>> @@ -29,6 +29,14 @@
>>   #define LPASS_CPU_I2S_SD0_1_2_MASK    GENMASK(2, 0)
>>   #define LPASS_CPU_I2S_SD0_1_2_3_MASK    GENMASK(3, 0)
>>   +/*
>> + * Channel maps for Quad channel playbacks on MI2S Secondary
>> + */
>> +static struct snd_pcm_chmap_elem lpass_quad_chmaps = {
>> +        .channels = 4,
>> +        .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_RL,
>> +                SNDRV_CHMAP_FR, SNDRV_CHMAP_RR }
>
> AFAIU, You would need an empty entry at the end of this list if not we
> will endup in a dereferencing memory sitting right next to this array
> which will lead to random memory corruptions...
>
> Have a look at some of the examples in existing codec drivers.
>
> Other than that patch looks good.
>
> --srini
Okay. Will change accordingly.
>
>
>> +};
>>   static int lpass_cpu_init_i2sctl_bitfields(struct device *dev,
>>               struct lpaif_i2sctl *i2sctl, struct regmap *map)
>>   {
>> @@ -324,6 +332,25 @@ const struct snd_soc_dai_ops
>> asoc_qcom_lpass_cpu_dai_ops = {
>>   };
>>   EXPORT_SYMBOL_GPL(asoc_qcom_lpass_cpu_dai_ops);
>>   +int lpass_cpu_pcm_new(struct snd_soc_pcm_runtime *rtd,
>> +                struct snd_soc_dai *dai)
>> +{
>> +    int ret;
>> +    struct snd_soc_dai_driver *drv = dai->driver;
>> +    struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
>> +
>> +    if (drvdata->mi2s_playback_sd_mode[dai->id] ==
>> LPAIF_I2SCTL_MODE_QUAD01) {
>> +        ret =  snd_pcm_add_chmap_ctls(rtd->pcm,
>> SNDRV_PCM_STREAM_PLAYBACK,
>> +                &lpass_quad_chmaps, drv->playback.channels_max, 0,
>> +                NULL);
>> +        if (ret < 0)
>> +            return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(lpass_cpu_pcm_new);
>> +
>>   int asoc_qcom_lpass_cpu_dai_probe(struct snd_soc_dai *dai)
>>   {
>>       struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
>> @@ -856,6 +883,11 @@ int asoc_qcom_lpass_cpu_platform_probe(struct
>> platform_device *pdev)
>>                   PTR_ERR(drvdata->mi2s_bit_clk[dai_id]));
>>               return PTR_ERR(drvdata->mi2s_bit_clk[dai_id]);
>>           }
>> +        if (drvdata->mi2s_playback_sd_mode[dai_id] ==
>> +            LPAIF_I2SCTL_MODE_QUAD01) {
>> + variant->dai_driver[dai_id].playback.channels_min = 4;
>> + variant->dai_driver[dai_id].playback.channels_max = 4;
>> +        }
>>       }
>>         /* Allocation for i2sctl regmap fields */
>> diff --git a/sound/soc/qcom/lpass-sc7180.c
>> b/sound/soc/qcom/lpass-sc7180.c
>> index 8c168d3c589e..77a556b27cf0 100644
>> --- a/sound/soc/qcom/lpass-sc7180.c
>> +++ b/sound/soc/qcom/lpass-sc7180.c
>> @@ -58,6 +58,7 @@ static struct snd_soc_dai_driver
>> sc7180_lpass_cpu_dai_driver[] = {
>>           },
>>           .probe    = &asoc_qcom_lpass_cpu_dai_probe,
>>           .ops    = &asoc_qcom_lpass_cpu_dai_ops,
>> +        .pcm_new = lpass_cpu_pcm_new,
>>       }, {
>>           .id = LPASS_DP_RX,
>>           .name = "Hdmi",
>> diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
>> index 83b2e08ade06..623ddccdafff 100644
>> --- a/sound/soc/qcom/lpass.h
>> +++ b/sound/soc/qcom/lpass.h
>> @@ -259,5 +259,7 @@ void asoc_qcom_lpass_cpu_platform_shutdown(struct
>> platform_device *pdev);
>>   int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev);
>>   int asoc_qcom_lpass_cpu_dai_probe(struct snd_soc_dai *dai);
>>   extern const struct snd_soc_dai_ops asoc_qcom_lpass_cpu_dai_ops;
>> +int lpass_cpu_pcm_new(struct snd_soc_pcm_runtime *rtd,
>> +                struct snd_soc_dai *dai);
>>     #endif /* __LPASS_H__ */
>>
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.