| Message ID | 20231212095850.26482-1-lujianhua000@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC,v2,1/2] ASoC: qcom: sdw: Add TDM support | expand |
Thankyou for the patch, On 12/12/2023 09:58, Jianhua Lu wrote: > Add qcom TDM setup function to support TDM ports for qcom platform. > > Signed-off-by: Jianhua Lu <lujianhua000@gmail.com> > --- > Changes in v2: > 1. remove EXPORT_SYMBOL_GPL > 2. remove static modifier > > sound/soc/qcom/sdw.c | 63 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 63 insertions(+) > > diff --git a/sound/soc/qcom/sdw.c b/sound/soc/qcom/sdw.c > index 77dbe0c28b29..948abbd2e0e3 100644 > --- a/sound/soc/qcom/sdw.c > +++ b/sound/soc/qcom/sdw.c > @@ -4,6 +4,7 @@ > > #include <dt-bindings/sound/qcom,q6afe.h> > #include <linux/module.h> > +#include <sound/pcm_params.h> > #include <sound/soc.h> > #include "sdw.h" > > @@ -101,6 +102,65 @@ int qcom_snd_sdw_prepare(struct snd_pcm_substream *substream, > } > EXPORT_SYMBOL_GPL(qcom_snd_sdw_prepare); > > +int qcom_snd_tdm_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params) > +{ TBH, this should not be part of sdw.c file, its intended for more of soundwire specific helpers, pl consider moving this to common.c for now. Because, Not all old qcom platforms have soundwire controllers. > + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); > + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); > + unsigned int tdm_slot_offset[8] = { 0, 4, 8, 12, 16, 20, 24, 28 }; > + int channels, slot_width; > + int ret; > + > + switch (params_format(params)) { > + case SNDRV_PCM_FORMAT_S16_LE: > + slot_width = 16; > + break; > + case SNDRV_PCM_FORMAT_S24_LE: > + slot_width = 24; > + break; > + case SNDRV_PCM_FORMAT_S32_LE: > + slot_width = 32; > + break; > + default: > + dev_err(rtd->dev, "%s: invalid param format 0x%x\n", __func__, > + params_format(params)); > + return -EINVAL; > + } > + > + channels = params_channels(params); > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { > + ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0, 0x3, 8, slot_width); slot mask is always set to 2 channels in this case, should you not check the number of channels to determine the correct one? These magic number 0, 0x3, 8 seems to make the code unreadable, can you do something like this: snd_soc_dai_set_tdm_slot(cpu_dai, tx_mask, rx_mask, ARRAY_SIZE(tdm_slot_offset), slot_width); > + if (ret < 0) { > + dev_err(rtd->dev, "%s: failed to set tdm slot, err:%d\n", > + __func__, ret); > + return ret; > + } > + > + ret = snd_soc_dai_set_channel_map(cpu_dai, 0, NULL, channels, > + tdm_slot_offset); > + if (ret < 0) { > + dev_err(rtd->dev, "%s: failed to set channel map, err:%d\n", > + __func__, ret); > + return ret; > + } > + } else { > + ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0xf, 0, 8, slot_width); > + if (ret < 0) { > + dev_err(rtd->dev, "%s: failed to set tdm slot, err:%d\n", > + __func__, ret); > + return ret; > + } > + > + ret = snd_soc_dai_set_channel_map(cpu_dai, channels, > + tdm_slot_offset, 0, NULL); > + if (ret < 0) { > + dev_err(rtd->dev, "%s: failed to set channel map, err:%d\n", > + __func__, ret); > + return ret; > + } > + } Finally ./sound/soc/qcom/sdm845.c does have exactly same code, can you consider removing this and make use of this new helper in that file too. > +} > + > int qcom_snd_sdw_hw_params(struct snd_pcm_substream *substream, > struct snd_pcm_hw_params *params, > struct sdw_stream_runtime **psruntime) > @@ -125,6 +185,9 @@ int qcom_snd_sdw_hw_params(struct snd_pcm_substream *substream, > *psruntime = sruntime; > } > break; > + case PRIMARY_TDM_RX_0...QUINARY_TDM_TX_7: > + qcom_snd_tdm_hw_params(substream, params); > + break; > } > > return 0;
On Tue, Dec 12, 2023 at 11:47:36AM +0000, Srinivas Kandagatla wrote: > > +int qcom_snd_tdm_hw_params(struct snd_pcm_substream *substream, > > + struct snd_pcm_hw_params *params) > > +{ > > TBH, this should not be part of sdw.c file, its intended for more of > soundwire specific helpers, pl consider moving this to common.c for now. > Because, Not all old qcom platforms have soundwire controllers. Acked. > > > + ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0, 0x3, 8, slot_width); > > slot mask is always set to 2 channels in this case, should you not check > the number of channels to determine the correct one? > > > These magic number 0, 0x3, 8 seems to make the code unreadable, can you > do something like this: > snd_soc_dai_set_tdm_slot(cpu_dai, tx_mask, rx_mask, > ARRAY_SIZE(tdm_slot_offset), slot_width); Acked. > > > + } > > + } > Finally ./sound/soc/qcom/sdm845.c does have exactly same code, can you > consider removing this and make use of this new helper in that file too. Acked. Thanks for your reveiw very much, I will do it in patch v3.
diff --git a/sound/soc/qcom/sdw.c b/sound/soc/qcom/sdw.c index 77dbe0c28b29..948abbd2e0e3 100644 --- a/sound/soc/qcom/sdw.c +++ b/sound/soc/qcom/sdw.c @@ -4,6 +4,7 @@ #include <dt-bindings/sound/qcom,q6afe.h> #include <linux/module.h> +#include <sound/pcm_params.h> #include <sound/soc.h> #include "sdw.h" @@ -101,6 +102,65 @@ int qcom_snd_sdw_prepare(struct snd_pcm_substream *substream, } EXPORT_SYMBOL_GPL(qcom_snd_sdw_prepare); +int qcom_snd_tdm_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream); + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0); + unsigned int tdm_slot_offset[8] = { 0, 4, 8, 12, 16, 20, 24, 28 }; + int channels, slot_width; + int ret; + + switch (params_format(params)) { + case SNDRV_PCM_FORMAT_S16_LE: + slot_width = 16; + break; + case SNDRV_PCM_FORMAT_S24_LE: + slot_width = 24; + break; + case SNDRV_PCM_FORMAT_S32_LE: + slot_width = 32; + break; + default: + dev_err(rtd->dev, "%s: invalid param format 0x%x\n", __func__, + params_format(params)); + return -EINVAL; + } + + channels = params_channels(params); + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0, 0x3, 8, slot_width); + if (ret < 0) { + dev_err(rtd->dev, "%s: failed to set tdm slot, err:%d\n", + __func__, ret); + return ret; + } + + ret = snd_soc_dai_set_channel_map(cpu_dai, 0, NULL, channels, + tdm_slot_offset); + if (ret < 0) { + dev_err(rtd->dev, "%s: failed to set channel map, err:%d\n", + __func__, ret); + return ret; + } + } else { + ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0xf, 0, 8, slot_width); + if (ret < 0) { + dev_err(rtd->dev, "%s: failed to set tdm slot, err:%d\n", + __func__, ret); + return ret; + } + + ret = snd_soc_dai_set_channel_map(cpu_dai, channels, + tdm_slot_offset, 0, NULL); + if (ret < 0) { + dev_err(rtd->dev, "%s: failed to set channel map, err:%d\n", + __func__, ret); + return ret; + } + } +} + int qcom_snd_sdw_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct sdw_stream_runtime **psruntime) @@ -125,6 +185,9 @@ int qcom_snd_sdw_hw_params(struct snd_pcm_substream *substream, *psruntime = sruntime; } break; + case PRIMARY_TDM_RX_0...QUINARY_TDM_TX_7: + qcom_snd_tdm_hw_params(substream, params); + break; } return 0;
Add qcom TDM setup function to support TDM ports for qcom platform. Signed-off-by: Jianhua Lu <lujianhua000@gmail.com> --- Changes in v2: 1. remove EXPORT_SYMBOL_GPL 2. remove static modifier sound/soc/qcom/sdw.c | 63 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)