On Thu, Dec 3, 2020 at 4:23 AM Nicolin Chen <nicoleotsuka@xxxxxxxxx> wrote:
>
> 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@xxxxxxx>
> > ---
> > 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?
The master clock always 256 * rate, when slot_width=32. so use
the 8 * slot_width. will add 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.
ok.
>
> > +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
>
Good catch, will update it.
> > + data->card.num_links = 1;
> > + data->card.dai_link = &data->dai;
> > +
> > + platform_set_drvdata(pdev, &data->card);
>
> Why pass card pointer?
Seems it duplicates with dev_set_drvdata(card->dev, card);
in snd_soc_register_card. will remove it.
best regards
wang shengjiu
