Re: [PATCH 2/2] ASoC: fsl: Add imx-hdmi machine driver

On Fri, Nov 27, 2020 at 01:30:21PM +0800, Shengjiu Wang wrote:

The driver is initially designed for sound card using HDMI interface on i.MX platform. There is internal HDMI IP or external HDMI modules connect with SAI or AUD2HTX interface. It supports both transmitter and receiver devices.

Signed-off-by: Shengjiu Wang shengjiu.wang@nxp.com

sound/soc/fsl/Kconfig | 12 ++ sound/soc/fsl/Makefile | 2 + sound/soc/fsl/imx-hdmi.c | 235 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 249 insertions(+) create mode 100644 sound/soc/fsl/imx-hdmi.c

diff --git a/sound/soc/fsl/imx-hdmi.c b/sound/soc/fsl/imx-hdmi.c new file mode 100644 index 000000000000..ac164514b1b2 --- /dev/null +++ b/sound/soc/fsl/imx-hdmi.c

+static int imx_hdmi_hw_params(struct snd_pcm_substream *substream,

• 	      struct snd_pcm_hw_params *params)
  

+{

• struct snd_soc_pcm_runtime *rtd = substream->private_data;
• struct imx_hdmi_data *data = snd_soc_card_get_drvdata(rtd->card);
• bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
• struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
• struct snd_soc_card *card = rtd->card;
• struct device *dev = card->dev;
• int ret;
• /* set cpu DAI configuration */
• ret = snd_soc_dai_set_sysclk(cpu_dai, data->cpu_priv.sysclk_id[tx],

• 		     8 * data->cpu_priv.slot_width * params_rate(params),
  

Looks like fixed 2 slots being used, judging by the set_tdm_slot call below. Then...why "8 *"? Probably need a line of comments?

• 		     tx ? SND_SOC_CLOCK_OUT : SND_SOC_CLOCK_IN);
  

• if (ret && ret != -ENOTSUPP) {

• dev_err(dev, "failed to set cpu sysclk: %d\n", ret);
  

• return ret;
  

• }
• ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0, 0, 2, data->cpu_priv.slot_width);

May have a local variable to cache slot_width.

+static int imx_hdmi_probe(struct platform_device *pdev)

• data->dai.name = "i.MX HDMI";
• data->dai.stream_name = "i.MX HDMI";
• data->dai.cpus->dai_name = dev_name(&cpu_pdev->dev);
• data->dai.platforms->of_node = cpu_np;
• data->dai.ops = &imx_hdmi_ops;
• data->dai.playback_only = true;
• data->dai.capture_only = false;
• data->dai.init = imx_hdmi_init;
• if (of_property_read_bool(np, "hdmi-out")) {

• data->dai.playback_only = true;
  

• data->dai.capture_only = false;
  

• data->dai.codecs->dai_name = "i2s-hifi";
  

• data->dai.codecs->name = "hdmi-audio-codec.1";
  

• data->dai.dai_fmt = data->dai_fmt |
  

• 		    SND_SOC_DAIFMT_NB_NF |
  

• 		    SND_SOC_DAIFMT_CBS_CFS;
  

• }
• if (of_property_read_bool(np, "hdmi-in")) {

• data->dai.playback_only = false;
  

• data->dai.capture_only = true;
  

• data->dai.codecs->dai_name = "i2s-hifi";
  

• data->dai.codecs->name = "hdmi-audio-codec.2";
  

• data->dai.dai_fmt = data->dai_fmt |
  

• 		    SND_SOC_DAIFMT_NB_NF |
  

• 		    SND_SOC_DAIFMT_CBM_CFM;
  

• }
• if ((data->dai.playback_only && data->dai.capture_only) ||

•    (!data->dai.playback_only && !data->dai.capture_only)) {
  

• dev_err(&pdev->dev, "Wrongly enable HDMI DAI link\n");
  

• goto fail;
  

• }

Seems that this condition check can never be true, given that: 1. By default: playback_only=true && capture_only=false 2. Conditionally overwritten: playback_only=true && capture_only=false 3. Conditionally overwritten: playback_only=false && capture_only=true

If I understand it correctly, probably should be something like: bool hdmi_out = of_property_read_bool(np, "hdmi-out"); bool hdmi_in = of_property_read_bool(np, "hdmi-in");

if ((hdmi_out && hdmi_in) || (!hdmi_out || !hdmi_in)) // "Invalid HDMI DAI link"; goto fail;

if (hdmi_out) { // ... } else if (hdmi_in) { // ... } else // No need of this line if two properties are exclusive

• data->card.num_links = 1;
• data->card.dai_link = &data->dai;
• platform_set_drvdata(pdev, &data->card);

Why pass card pointer?