diff mbox series

[1/2] ASoC: dt-bindings: imx-hdmi: Add binding doc for hdmi machine driver

Message ID 1606455021-18882-1-git-send-email-shengjiu.wang@nxp.com
State Superseded
Headers show
Series [1/2] ASoC: dt-bindings: imx-hdmi: Add binding doc for hdmi machine driver | expand

Checks

Context Check Description
robh/dt-meta-schema success
robh/checkpatch success

Commit Message

Shengjiu Wang Nov. 27, 2020, 5:30 a.m. UTC
Imx-hdmi is a new added machine driver for supporting hdmi devices
on i.MX platforms. There is HDMI IP or external HDMI modules connect
with SAI or AUD2HTX interface.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 .../bindings/sound/imx-audio-hdmi.yaml        | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/imx-audio-hdmi.yaml

Comments

Nicolin Chen Dec. 2, 2020, 8:19 p.m. UTC | #1
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?
Shengjiu Wang Dec. 3, 2020, 3:51 a.m. UTC | #2
On Thu, Dec 3, 2020 at 4:23 AM Nicolin Chen <nicoleotsuka@gmail.com> 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@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?

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
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sound/imx-audio-hdmi.yaml b/Documentation/devicetree/bindings/sound/imx-audio-hdmi.yaml
new file mode 100644
index 000000000000..d5474f83ac2c
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/imx-audio-hdmi.yaml
@@ -0,0 +1,52 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/imx-audio-hdmi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP i.MX audio complex with HDMI
+
+maintainers:
+  - Shengjiu Wang <shengjiu.wang@nxp.com>
+
+properties:
+  compatible:
+    enum:
+      - fsl,imx-audio-hdmi
+      - fsl,imx-audio-sii902x
+
+  model:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: User specified audio sound card name
+
+  audio-cpu:
+    description: The phandle of an CPU DAI controller
+
+  hdmi-out:
+    description: |
+      This is a boolean property. If present, the transmitting function
+      of HDMI will be enabled, indicating there's a physical HDMI out
+      connector or jack on the board or it's connecting to some other IP
+      block, such as an HDMI encoder or display-controller.
+
+  hdmi-in:
+    description: |
+      This is a boolean property. If present, the receiving function of
+      HDMI will be enabled, indicating there is a physical HDMI in
+      connector/jack on the board.
+
+required:
+  - compatible
+  - model
+  - audio-cpu
+
+additionalProperties: false
+
+examples:
+  - |
+    sound-hdmi {
+        compatible = "fsl,imx-audio-hdmi";
+        model = "audio-hdmi";
+        audio-cpu = <&aud2htx>;
+        hdmi-out;
+    };