[PATCH v3 0/6] drm/bridge: imx: Add HDMI PAI driver on i.MX8MP

[PATCH v3 0/6] drm/bridge: imx: Add HDMI PAI driver on i.MX8MP

[Shengjiu Wang]

The HDMI TX Parallel Audio Interface (HTX_PAI) is a digital module that
acts as the bridge between the Audio Subsystem to the HDMI TX Controller.

Add HDMI PAI driver on i.MX8MP to make HDMI audio function fully work.

changes in v3:
- add space and 'U' in asoundef.h
- add more commit message for binding doc commit
- add bitfield.h header for fixing build error

changes in v2:
- address some comments on commit messages
- add two more commits:
  add definitions for the bits in IEC958 subframe
  add API dw_hdmi_set_sample_iec958() for iec958 format
- use component helper in hdmi_pai and hdmi_tx driver
- use regmap in hdmi_pai driver.
- add clocks in binding doc

Shengjiu Wang (6):
  dt-bindings: display: imx: add HDMI PAI for i.MX8MP
  ALSA: Add definitions for the bits in IEC958 subframe
  drm/bridge: dw-hdmi: Add API dw_hdmi_to_plat_data() to get plat_data
  drm/bridge: dw-hdmi: Add API dw_hdmi_set_sample_iec958() for iec958
    format
  drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface
  arm64: dts: imx8mp: Add hdmi parallel audio interface node

 .../display/bridge/fsl,imx8mp-hdmi-tx.yaml    |  12 +
 .../display/imx/fsl,imx8mp-hdmi-pai.yaml      |  69 ++++++
 arch/arm64/boot/dts/freescale/imx8mp-evk.dts  |   4 +
 arch/arm64/boot/dts/freescale/imx8mp.dtsi     |  28 ++-
 drivers/gpu/drm/bridge/imx/Kconfig            |   8 +
 drivers/gpu/drm/bridge/imx/Makefile           |   1 +
 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c  | 205 ++++++++++++++++++
 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c   |  55 +++++
 .../drm/bridge/synopsys/dw-hdmi-gp-audio.c    |   5 +
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c     |  18 +-
 include/drm/bridge/dw_hdmi.h                  |  11 +-
 include/sound/asoundef.h                      |   9 +
 12 files changed, 422 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi-pai.yaml
 create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c

-- 
2.34.1

[PATCH v3 1/6] dt-bindings: display: imx: add HDMI PAI for i.MX8MP

[Shengjiu Wang]

Add binding for the i.MX8MP HDMI parallel Audio interface block.

The HDMI TX Parallel Audio Interface (HTX_PAI) is a digital module that
acts as the bridge between the Audio Subsystem to the HDMI TX Controller.
This IP block is found in the HDMI subsystem of the i.MX8MP SoC.

Aud2htx module in Audio Subsystem, HDMI PAI module and HDMI TX
Controller compose the HDMI audio pipeline.

In fsl,imx8mp-hdmi-tx.yaml, add port@2 that is linked to pai_to_hdmi_tx.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 .../display/bridge/fsl,imx8mp-hdmi-tx.yaml    | 12 ++++
 .../display/imx/fsl,imx8mp-hdmi-pai.yaml      | 69 +++++++++++++++++++
 2 files changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi-pai.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/fsl,imx8mp-hdmi-tx.yaml b/Documentation/devicetree/bindings/display/bridge/fsl,imx8mp-hdmi-tx.yaml
index 05442d437755..6211ab8bbb0e 100644
--- a/Documentation/devicetree/bindings/display/bridge/fsl,imx8mp-hdmi-tx.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/fsl,imx8mp-hdmi-tx.yaml
@@ -49,6 +49,10 @@ properties:
         $ref: /schemas/graph.yaml#/properties/port
         description: HDMI output port
 
+      port@2:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Parallel audio input port
+
     required:
       - port@0
       - port@1
@@ -98,5 +102,13 @@ examples:
                     remote-endpoint = <&hdmi0_con>;
                 };
             };
+
+            port@2 {
+                reg = <2>;
+
+                endpoint {
+                    remote-endpoint = <&pai_to_hdmi_tx>;
+                };
+            };
         };
     };
diff --git a/Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi-pai.yaml b/Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi-pai.yaml
new file mode 100644
index 000000000000..4f99682a308d
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi-pai.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/imx/fsl,imx8mp-hdmi-pai.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale i.MX8MP HDMI Parallel Audio Interface
+
+maintainers:
+  - Shengjiu Wang <shengjiu.wang@nxp.com>
+
+description:
+  The HDMI TX Parallel Audio Interface (HTX_PAI) is a bridge between the
+  Audio Subsystem to the HDMI TX Controller.
+
+properties:
+  compatible:
+    const: fsl,imx8mp-hdmi-pai
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: apb
+
+  power-domains:
+    maxItems: 1
+
+  port:
+    $ref: /schemas/graph.yaml#/properties/port
+    description: Output to the HDMI TX controller.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - power-domains
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8mp-clock.h>
+    #include <dt-bindings/power/imx8mp-power.h>
+
+    audio-bridge@32fc4800 {
+        compatible = "fsl,imx8mp-hdmi-pai";
+        reg = <0x32fc4800 0x800>;
+        interrupt-parent = <&irqsteer_hdmi>;
+        interrupts = <14>;
+        clocks = <&clk IMX8MP_CLK_HDMI_APB>;
+        clock-names = "apb";
+        power-domains = <&hdmi_blk_ctrl IMX8MP_HDMIBLK_PD_PAI>;
+
+        port {
+            pai_to_hdmi_tx: endpoint {
+                remote-endpoint = <&hdmi_tx_from_pai>;
+            };
+        };
+    };
-- 
2.34.1

[PATCH v3 2/6] ALSA: Add definitions for the bits in IEC958 subframe

[Shengjiu Wang]

The IEC958 subframe format SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE are used
in HDMI and DisplayPort to describe the audio stream, but hardware device
may need to reorder the IEC958 bits for internal transmission, so need
these standard bits definitions for IEC958 subframe format.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/asoundef.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/sound/asoundef.h b/include/sound/asoundef.h
index 09b2c3dffb30..c4a929d4fd51 100644
--- a/include/sound/asoundef.h
+++ b/include/sound/asoundef.h
@@ -12,6 +12,15 @@
  *        Digital audio interface					    *
  *                                                                          *
  ****************************************************************************/
+/* IEC958 subframe format */
+#define IEC958_SUBFRAME_PREAMBLE_MASK	(0xfU)
+#define IEC958_SUBFRAME_AUXILIARY_MASK	(0xfU << 4)
+#define IEC958_SUBFRAME_SAMPLE_24_MASK	(0xffffffU << 4)
+#define IEC958_SUBFRAME_SAMPLE_20_MASK	(0xfffffU << 8)
+#define IEC958_SUBFRAME_VALIDITY	(0x1U << 28)
+#define IEC958_SUBFRAME_USER_DATA	(0x1U << 29)
+#define IEC958_SUBFRAME_CHANNEL_STATUS	(0x1U << 30)
+#define IEC958_SUBFRAME_PARITY		(0x1U << 31)
 
 /* AES/IEC958 channel status bits */
 #define IEC958_AES0_PROFESSIONAL	(1<<0)	/* 0 = consumer, 1 = professional */
-- 
2.34.1

[PATCH v3 3/6] drm/bridge: dw-hdmi: Add API dw_hdmi_to_plat_data() to get plat_data

[Shengjiu Wang]

Add API dw_hdmi_to_plat_data() to fetch plat_data because audio device
driver needs it to enable(disable)_audio().

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 6 ++++++
 include/drm/bridge/dw_hdmi.h              | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 206b099a35e9..8d096b569cf1 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -198,6 +198,12 @@ struct dw_hdmi {
 	enum drm_connector_status last_connector_result;
 };
 
+const struct dw_hdmi_plat_data *dw_hdmi_to_plat_data(struct dw_hdmi *hdmi)
+{
+	return hdmi->plat_data;
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_to_plat_data);
+
 #define HDMI_IH_PHY_STAT0_RX_SENSE \
 	(HDMI_IH_PHY_STAT0_RX_SENSE0 | HDMI_IH_PHY_STAT0_RX_SENSE1 | \
 	 HDMI_IH_PHY_STAT0_RX_SENSE2 | HDMI_IH_PHY_STAT0_RX_SENSE3)
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 6a46baa0737c..b8fc4fdf5a21 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -208,4 +208,6 @@ void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data);
 
 bool dw_hdmi_bus_fmt_is_420(struct dw_hdmi *hdmi);
 
+const struct dw_hdmi_plat_data *dw_hdmi_to_plat_data(struct dw_hdmi *hdmi);
+
 #endif /* __IMX_HDMI_H__ */
-- 
2.34.1

[PATCH v3 4/6] drm/bridge: dw-hdmi: Add API dw_hdmi_set_sample_iec958() for iec958 format

[Shengjiu Wang]

Add API dw_hdmi_set_sample_iec958() for iec958 format because audio device
driver needs iec958 information to configure this specific setting.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-gp-audio.c |  5 +++++
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          | 12 +++++++++++-
 include/drm/bridge/dw_hdmi.h                       |  3 ++-
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-gp-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-gp-audio.c
index ab18f9a3bf23..df7a37eb47f4 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-gp-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-gp-audio.c
@@ -90,6 +90,11 @@ static int audio_hw_params(struct device *dev,  void *data,
 				   params->iec.status[0] & IEC958_AES0_NONAUDIO);
 	dw_hdmi_set_sample_width(dw->data.hdmi, params->sample_width);
 
+	if (daifmt->bit_fmt == SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE)
+		dw_hdmi_set_sample_iec958(dw->data.hdmi, 1);
+	else
+		dw_hdmi_set_sample_iec958(dw->data.hdmi, 0);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 8d096b569cf1..3b77e73ac0ea 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -177,6 +177,7 @@ struct dw_hdmi {
 
 	spinlock_t audio_lock;
 	struct mutex audio_mutex;
+	unsigned int sample_iec958;
 	unsigned int sample_non_pcm;
 	unsigned int sample_width;
 	unsigned int sample_rate;
@@ -718,6 +719,14 @@ void dw_hdmi_set_sample_non_pcm(struct dw_hdmi *hdmi, unsigned int non_pcm)
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_non_pcm);
 
+void dw_hdmi_set_sample_iec958(struct dw_hdmi *hdmi, unsigned int iec958)
+{
+	mutex_lock(&hdmi->audio_mutex);
+	hdmi->sample_iec958 = iec958;
+	mutex_unlock(&hdmi->audio_mutex);
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_iec958);
+
 void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate)
 {
 	mutex_lock(&hdmi->audio_mutex);
@@ -849,7 +858,8 @@ static void dw_hdmi_gp_audio_enable(struct dw_hdmi *hdmi)
 				    hdmi->channels,
 				    hdmi->sample_width,
 				    hdmi->sample_rate,
-				    hdmi->sample_non_pcm);
+				    hdmi->sample_non_pcm,
+				    hdmi->sample_iec958);
 }
 
 static void dw_hdmi_gp_audio_disable(struct dw_hdmi *hdmi)
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index b8fc4fdf5a21..095cdd9b7424 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -145,7 +145,7 @@ struct dw_hdmi_plat_data {
 
 	/* Platform-specific audio enable/disable (optional) */
 	void (*enable_audio)(struct dw_hdmi *hdmi, int channel,
-			     int width, int rate, int non_pcm);
+			     int width, int rate, int non_pcm, int iec958);
 	void (*disable_audio)(struct dw_hdmi *hdmi);
 
 	/* Vendor PHY support */
@@ -179,6 +179,7 @@ void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense);
 int dw_hdmi_set_plugged_cb(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb fn,
 			   struct device *codec_dev);
 void dw_hdmi_set_sample_non_pcm(struct dw_hdmi *hdmi, unsigned int non_pcm);
+void dw_hdmi_set_sample_iec958(struct dw_hdmi *hdmi, unsigned int iec958);
 void dw_hdmi_set_sample_width(struct dw_hdmi *hdmi, unsigned int width);
 void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
 void dw_hdmi_set_channel_count(struct dw_hdmi *hdmi, unsigned int cnt);
-- 
2.34.1

[PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Shengjiu Wang]

The HDMI TX Parallel Audio Interface (HTX_PAI) is a digital module that
acts as the bridge between the Audio Subsystem to the HDMI TX Controller.
This IP block is found in the HDMI subsystem of the i.MX8MP SoC.

Data received from the audio subsystem can have an arbitrary component
ordering. The HTX_PAI block has integrated muxing options to select which
sections of the 32-bit input data word will be mapped to each IEC60958
field. The HTX_PAI_FIELD_CTRL register contains mux selects to
individually select P,C,U,V,Data, and Preamble.

Use component helper that imx8mp-hdmi-tx will be aggregate driver,
imx8mp-hdmi-pai will be component driver, then imx8mp-hdmi-pai can use
bind() ops to get the plat_data from imx8mp-hdmi-tx device.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 drivers/gpu/drm/bridge/imx/Kconfig           |   8 +
 drivers/gpu/drm/bridge/imx/Makefile          |   1 +
 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c | 205 +++++++++++++++++++
 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c  |  55 +++++
 include/drm/bridge/dw_hdmi.h                 |   6 +
 5 files changed, 275 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c

diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig
index 9a480c6abb85..6c1a8bc5d4a0 100644
--- a/drivers/gpu/drm/bridge/imx/Kconfig
+++ b/drivers/gpu/drm/bridge/imx/Kconfig
@@ -18,12 +18,20 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE
 	depends on OF
 	depends on COMMON_CLK
 	select DRM_DW_HDMI
+	imply DRM_IMX8MP_HDMI_PAI
 	imply DRM_IMX8MP_HDMI_PVI
 	imply PHY_FSL_SAMSUNG_HDMI_PHY
 	help
 	  Choose this to enable support for the internal HDMI encoder found
 	  on the i.MX8MP SoC.
 
+config DRM_IMX8MP_HDMI_PAI
+	tristate "Freescale i.MX8MP HDMI PAI bridge support"
+	depends on OF
+	help
+	  Choose this to enable support for the internal HDMI TX Parallel
+	  Audio Interface found on the Freescale i.MX8MP SoC.
+
 config DRM_IMX8MP_HDMI_PVI
 	tristate "Freescale i.MX8MP HDMI PVI bridge support"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile
index dd5d48584806..8d01fda25451 100644
--- a/drivers/gpu/drm/bridge/imx/Makefile
+++ b/drivers/gpu/drm/bridge/imx/Makefile
@@ -1,6 +1,7 @@
 obj-$(CONFIG_DRM_IMX_LDB_HELPER) += imx-ldb-helper.o
 obj-$(CONFIG_DRM_IMX_LEGACY_BRIDGE) += imx-legacy-bridge.o
 obj-$(CONFIG_DRM_IMX8MP_DW_HDMI_BRIDGE) += imx8mp-hdmi-tx.o
+obj-$(CONFIG_DRM_IMX8MP_HDMI_PAI) += imx8mp-hdmi-pai.o
 obj-$(CONFIG_DRM_IMX8MP_HDMI_PVI) += imx8mp-hdmi-pvi.o
 obj-$(CONFIG_DRM_IMX8QM_LDB) += imx8qm-ldb.o
 obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
new file mode 100644
index 000000000000..9002974073ca
--- /dev/null
+++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
@@ -0,0 +1,205 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2025 NXP
+ */
+
+#include <linux/bitfield.h>
+#include <linux/component.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <drm/bridge/dw_hdmi.h>
+#include <sound/asoundef.h>
+
+#define HTX_PAI_CTRL			0x00
+#define   ENABLE			BIT(0)
+
+#define HTX_PAI_CTRL_EXT		0x04
+#define   WTMK_HIGH_MASK		GENMASK(31, 24)
+#define   WTMK_LOW_MASK			GENMASK(23, 16)
+#define   NUM_CH_MASK			GENMASK(10, 8)
+#define   WTMK_HIGH(n)			FIELD_PREP(WTMK_HIGH_MASK, (n))
+#define   WTMK_LOW(n)			FIELD_PREP(WTMK_LOW_MASK, (n))
+
+#define HTX_PAI_FIELD_CTRL		0x08
+#define   B_FILT			BIT(31)
+#define   PARITY_EN			BIT(30)
+#define   END_SEL			BIT(29)
+#define   PRE_SEL			GENMASK(28, 24)
+#define   D_SEL				GENMASK(23, 20)
+#define   V_SEL				GENMASK(19, 15)
+#define   U_SEL				GENMASK(14, 10)
+#define   C_SEL				GENMASK(9, 5)
+#define   P_SEL				GENMASK(4, 0)
+
+#define HTX_PAI_STAT			0x0c
+#define HTX_PAI_IRQ_NOMASK		0x10
+#define HTX_PAI_IRQ_MASKED		0x14
+#define HTX_PAI_IRQ_MASK		0x18
+
+struct imx8mp_hdmi_pai {
+	struct device	*dev;
+	struct regmap	*regmap;
+};
+
+static void imx8mp_hdmi_pai_enable(struct dw_hdmi *dw_hdmi, int channel,
+				   int width, int rate, int non_pcm,
+				   int iec958)
+{
+	const struct dw_hdmi_plat_data *pdata = dw_hdmi_to_plat_data(dw_hdmi);
+	struct imx8mp_hdmi_pai *hdmi_pai = (struct imx8mp_hdmi_pai *)pdata->priv_audio;
+	int val;
+
+	/* PAI set control extended */
+	val =  WTMK_HIGH(3) | WTMK_LOW(3);
+	val |= FIELD_PREP(NUM_CH_MASK, channel - 1);
+	regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL_EXT, val);
+
+	/* IEC60958 format */
+	if (iec958) {
+		val = FIELD_PREP_CONST(P_SEL,
+				       __bf_shf(IEC958_SUBFRAME_PARITY));
+		val |= FIELD_PREP_CONST(C_SEL,
+					__bf_shf(IEC958_SUBFRAME_CHANNEL_STATUS));
+		val |= FIELD_PREP_CONST(U_SEL,
+					__bf_shf(IEC958_SUBFRAME_USER_DATA));
+		val |= FIELD_PREP_CONST(V_SEL,
+					__bf_shf(IEC958_SUBFRAME_VALIDITY));
+		val |= FIELD_PREP_CONST(D_SEL,
+					__bf_shf(IEC958_SUBFRAME_SAMPLE_24_MASK));
+		val |= FIELD_PREP_CONST(PRE_SEL,
+					__bf_shf(IEC958_SUBFRAME_PREAMBLE_MASK));
+	} else {
+		/* PCM choose 24bit*/
+		val = FIELD_PREP(D_SEL, width - 24);
+	}
+
+	regmap_write(hdmi_pai->regmap, HTX_PAI_FIELD_CTRL, val);
+
+	/* PAI start running */
+	regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL, ENABLE);
+}
+
+static void imx8mp_hdmi_pai_disable(struct dw_hdmi *dw_hdmi)
+{
+	const struct dw_hdmi_plat_data *pdata = dw_hdmi_to_plat_data(dw_hdmi);
+	struct imx8mp_hdmi_pai *hdmi_pai = (struct imx8mp_hdmi_pai *)pdata->priv_audio;
+
+	/* Stop PAI */
+	regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL, 0);
+}
+
+static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
+{
+	struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
+	struct imx8mp_hdmi_pai *hdmi_pai;
+
+	hdmi_pai = dev_get_drvdata(dev);
+
+	plat_data->enable_audio = imx8mp_hdmi_pai_enable;
+	plat_data->disable_audio = imx8mp_hdmi_pai_disable;
+	plat_data->priv_audio = hdmi_pai;
+
+	return 0;
+}
+
+static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
+{
+	struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
+
+	plat_data->enable_audio = NULL;
+	plat_data->disable_audio = NULL;
+	plat_data->priv_audio = NULL;
+}
+
+static const struct component_ops imx8mp_hdmi_pai_ops = {
+	.bind   = imx8mp_hdmi_pai_bind,
+	.unbind = imx8mp_hdmi_pai_unbind,
+};
+
+static bool imx8mp_hdmi_pai_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case HTX_PAI_IRQ_NOMASK:
+	case HTX_PAI_IRQ_MASKED:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool imx8mp_hdmi_pai_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case HTX_PAI_CTRL:
+	case HTX_PAI_CTRL_EXT:
+	case HTX_PAI_FIELD_CTRL:
+	case HTX_PAI_IRQ_NOMASK:
+	case HTX_PAI_IRQ_MASKED:
+	case HTX_PAI_IRQ_MASK:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config imx8mp_hdmi_pai_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.max_register = HTX_PAI_IRQ_MASK,
+	.volatile_reg = imx8mp_hdmi_pai_volatile_reg,
+	.writeable_reg = imx8mp_hdmi_pai_writeable_reg,
+};
+
+static int imx8mp_hdmi_pai_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx8mp_hdmi_pai *hdmi_pai;
+	struct resource *res;
+	void __iomem *base;
+
+	hdmi_pai = devm_kzalloc(dev, sizeof(*hdmi_pai), GFP_KERNEL);
+	if (!hdmi_pai)
+		return -ENOMEM;
+
+	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	hdmi_pai->dev = dev;
+
+	hdmi_pai->regmap = devm_regmap_init_mmio(dev, base, &imx8mp_hdmi_pai_regmap_config);
+	if (IS_ERR(hdmi_pai->regmap)) {
+		dev_err(dev, "regmap init failed\n");
+		return PTR_ERR(hdmi_pai->regmap);
+	}
+
+	dev_set_drvdata(dev, hdmi_pai);
+
+	return component_add(dev, &imx8mp_hdmi_pai_ops);
+}
+
+static void imx8mp_hdmi_pai_remove(struct platform_device *pdev)
+{
+	component_del(&pdev->dev, &imx8mp_hdmi_pai_ops);
+}
+
+static const struct of_device_id imx8mp_hdmi_pai_of_table[] = {
+	{ .compatible = "fsl,imx8mp-hdmi-pai" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx8mp_hdmi_pai_of_table);
+
+static struct platform_driver imx8mp_hdmi_pai_platform_driver = {
+	.probe		= imx8mp_hdmi_pai_probe,
+	.remove		= imx8mp_hdmi_pai_remove,
+	.driver		= {
+		.name	= "imx8mp-hdmi-pai",
+		.of_match_table = imx8mp_hdmi_pai_of_table,
+	},
+};
+module_platform_driver(imx8mp_hdmi_pai_platform_driver);
+
+MODULE_DESCRIPTION("i.MX8MP HDMI PAI driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
index 1e7a789ec289..ee08084d2394 100644
--- a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
+++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
@@ -5,11 +5,13 @@
  */
 
 #include <linux/clk.h>
+#include <linux/component.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <drm/bridge/dw_hdmi.h>
 #include <drm/drm_modes.h>
+#include <drm/drm_of.h>
 
 struct imx8mp_hdmi {
 	struct dw_hdmi_plat_data plat_data;
@@ -79,11 +81,46 @@ static const struct dw_hdmi_phy_ops imx8mp_hdmi_phy_ops = {
 	.update_hpd	= dw_hdmi_phy_update_hpd,
 };
 
+static int imx8mp_dw_hdmi_bind(struct device *dev)
+{
+	struct dw_hdmi_plat_data *plat_data;
+	struct imx8mp_hdmi *hdmi;
+	int ret;
+
+	hdmi = dev_get_drvdata(dev);
+	plat_data = &hdmi->plat_data;
+
+	ret = component_bind_all(dev, plat_data);
+	if (ret)
+		return dev_err_probe(dev, ret, "component_bind_all failed!\n");
+
+	return 0;
+}
+
+static void imx8mp_dw_hdmi_unbind(struct device *dev)
+{
+	struct dw_hdmi_plat_data *plat_data;
+	struct imx8mp_hdmi *hdmi;
+
+	hdmi = dev_get_drvdata(dev);
+	plat_data = &hdmi->plat_data;
+
+	component_unbind_all(dev, plat_data);
+}
+
+static const struct component_master_ops imx8mp_dw_hdmi_ops = {
+	.bind   = imx8mp_dw_hdmi_bind,
+	.unbind = imx8mp_dw_hdmi_unbind,
+};
+
 static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct dw_hdmi_plat_data *plat_data;
+	struct component_match *match;
+	struct device_node *remote;
 	struct imx8mp_hdmi *hdmi;
+	int ret;
 
 	hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
 	if (!hdmi)
@@ -108,6 +145,22 @@ static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, hdmi);
 
+	/* port@2 is for hdmi_pai device */
+	remote = of_graph_get_remote_node(pdev->dev.of_node, 2, 0);
+	if (remote && of_device_is_available(remote)) {
+		drm_of_component_match_add(dev, &match, component_compare_of, remote);
+
+		of_node_put(remote);
+
+		ret = component_master_add_with_match(dev, &imx8mp_dw_hdmi_ops, match);
+		if (ret)
+			dev_warn(dev, "Unable to register aggregate driver\n");
+		/*
+		 * This audio function is optional for avoid blocking display.
+		 * So just print warning message and no error is returned.
+		 */
+	}
+
 	return 0;
 }
 
@@ -115,6 +168,8 @@ static void imx8mp_dw_hdmi_remove(struct platform_device *pdev)
 {
 	struct imx8mp_hdmi *hdmi = platform_get_drvdata(pdev);
 
+	component_master_del(&pdev->dev, &imx8mp_dw_hdmi_ops);
+
 	dw_hdmi_remove(hdmi->dw_hdmi);
 }
 
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 095cdd9b7424..336f062e1f9d 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -143,6 +143,12 @@ struct dw_hdmi_plat_data {
 					   const struct drm_display_info *info,
 					   const struct drm_display_mode *mode);
 
+	/*
+	 * priv_audio is specially used for additional audio device to get
+	 * driver data through this dw_hdmi_plat_data.
+	 */
+	void *priv_audio;
+
 	/* Platform-specific audio enable/disable (optional) */
 	void (*enable_audio)(struct dw_hdmi *hdmi, int channel,
 			     int width, int rate, int non_pcm, int iec958);
-- 
2.34.1

[PATCH v3 6/6] arm64: dts: imx8mp: Add hdmi parallel audio interface node

[Shengjiu Wang]

The HDMI TX Parallel Audio Interface (HTX_PAI) is a bridge between the
Audio Subsystem to the HDMI TX Controller.

Shrink register map size of hdmi_pvi to avoid overlapped hdmi_pai device.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mp-evk.dts |  4 +++
 arch/arm64/boot/dts/freescale/imx8mp.dtsi    | 28 +++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
index c0cc5611048e..cc9351a5bd65 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
@@ -442,6 +442,10 @@ &flexcan2 {
 	status = "disabled";/* can2 pin conflict with pdm */
 };
 
+&hdmi_pai {
+	status = "okay";
+};
+
 &hdmi_pvi {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 841d155685ee..00d8474bd1b1 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -2066,7 +2066,7 @@ irqsteer_hdmi: interrupt-controller@32fc2000 {
 
 			hdmi_pvi: display-bridge@32fc4000 {
 				compatible = "fsl,imx8mp-hdmi-pvi";
-				reg = <0x32fc4000 0x1000>;
+				reg = <0x32fc4000 0x800>;
 				interrupt-parent = <&irqsteer_hdmi>;
 				interrupts = <12>;
 				power-domains = <&hdmi_blk_ctrl IMX8MP_HDMIBLK_PD_PVI>;
@@ -2092,6 +2092,24 @@ pvi_to_hdmi_tx: endpoint {
 				};
 			};
 
+			hdmi_pai: audio-bridge@32fc4800 {
+				compatible = "fsl,imx8mp-hdmi-pai";
+				reg = <0x32fc4800 0x800>;
+				interrupt-parent = <&irqsteer_hdmi>;
+				interrupts = <14>;
+				clocks = <&clk IMX8MP_CLK_HDMI_APB>;
+				clock-names = "apb";
+				power-domains = <&hdmi_blk_ctrl IMX8MP_HDMIBLK_PD_PAI>;
+				status = "disabled";
+
+				port {
+
+					pai_to_hdmi_tx: endpoint {
+						remote-endpoint = <&hdmi_tx_from_pai>;
+					};
+				};
+			};
+
 			lcdif3: display-controller@32fc6000 {
 				compatible = "fsl,imx8mp-lcdif";
 				reg = <0x32fc6000 0x1000>;
@@ -2143,6 +2161,14 @@ port@1 {
 						reg = <1>;
 						/* Point endpoint to the HDMI connector */
 					};
+
+					port@2 {
+						reg = <2>;
+
+						hdmi_tx_from_pai: endpoint {
+							remote-endpoint = <&pai_to_hdmi_tx>;
+						};
+					};
 				};
 			};
 
-- 
2.34.1

Re: [PATCH v3 1/6] dt-bindings: display: imx: add HDMI PAI for i.MX8MP

[Krzysztof Kozlowski]

On Mon, Aug 04, 2025 at 06:47:17PM +0800, Shengjiu Wang wrote:
> Add binding for the i.MX8MP HDMI parallel Audio interface block.
> 
> The HDMI TX Parallel Audio Interface (HTX_PAI) is a digital module that
> acts as the bridge between the Audio Subsystem to the HDMI TX Controller.
> This IP block is found in the HDMI subsystem of the i.MX8MP SoC.
> 
> Aud2htx module in Audio Subsystem, HDMI PAI module and HDMI TX
> Controller compose the HDMI audio pipeline.
> 
> In fsl,imx8mp-hdmi-tx.yaml, add port@2 that is linked to pai_to_hdmi_tx.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  .../display/bridge/fsl,imx8mp-hdmi-tx.yaml    | 12 ++++
>  .../display/imx/fsl,imx8mp-hdmi-pai.yaml      | 69 +++++++++++++++++++
>  2 files changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8mp-hdmi-pai.yaml

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Alexander Stein]

Hi,

Am Montag, 4. August 2025, 12:47:21 CEST schrieb Shengjiu Wang:
> The HDMI TX Parallel Audio Interface (HTX_PAI) is a digital module that
> acts as the bridge between the Audio Subsystem to the HDMI TX Controller.
> This IP block is found in the HDMI subsystem of the i.MX8MP SoC.
> 
> Data received from the audio subsystem can have an arbitrary component
> ordering. The HTX_PAI block has integrated muxing options to select which
> sections of the 32-bit input data word will be mapped to each IEC60958
> field. The HTX_PAI_FIELD_CTRL register contains mux selects to
> individually select P,C,U,V,Data, and Preamble.
> 
> Use component helper that imx8mp-hdmi-tx will be aggregate driver,
> imx8mp-hdmi-pai will be component driver, then imx8mp-hdmi-pai can use
> bind() ops to get the plat_data from imx8mp-hdmi-tx device.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  drivers/gpu/drm/bridge/imx/Kconfig           |   8 +
>  drivers/gpu/drm/bridge/imx/Makefile          |   1 +
>  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c | 205 +++++++++++++++++++
>  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c  |  55 +++++
>  include/drm/bridge/dw_hdmi.h                 |   6 +
>  5 files changed, 275 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> 
> diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig
> index 9a480c6abb85..6c1a8bc5d4a0 100644
> --- a/drivers/gpu/drm/bridge/imx/Kconfig
> +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> @@ -18,12 +18,20 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE
>  	depends on OF
>  	depends on COMMON_CLK
>  	select DRM_DW_HDMI
> +	imply DRM_IMX8MP_HDMI_PAI
>  	imply DRM_IMX8MP_HDMI_PVI
>  	imply PHY_FSL_SAMSUNG_HDMI_PHY
>  	help
>  	  Choose this to enable support for the internal HDMI encoder found
>  	  on the i.MX8MP SoC.
>  
> +config DRM_IMX8MP_HDMI_PAI
> +	tristate "Freescale i.MX8MP HDMI PAI bridge support"
> +	depends on OF
> +	help
> +	  Choose this to enable support for the internal HDMI TX Parallel
> +	  Audio Interface found on the Freescale i.MX8MP SoC.
> +
>  config DRM_IMX8MP_HDMI_PVI
>  	tristate "Freescale i.MX8MP HDMI PVI bridge support"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile
> index dd5d48584806..8d01fda25451 100644
> --- a/drivers/gpu/drm/bridge/imx/Makefile
> +++ b/drivers/gpu/drm/bridge/imx/Makefile
> @@ -1,6 +1,7 @@
>  obj-$(CONFIG_DRM_IMX_LDB_HELPER) += imx-ldb-helper.o
>  obj-$(CONFIG_DRM_IMX_LEGACY_BRIDGE) += imx-legacy-bridge.o
>  obj-$(CONFIG_DRM_IMX8MP_DW_HDMI_BRIDGE) += imx8mp-hdmi-tx.o
> +obj-$(CONFIG_DRM_IMX8MP_HDMI_PAI) += imx8mp-hdmi-pai.o
>  obj-$(CONFIG_DRM_IMX8MP_HDMI_PVI) += imx8mp-hdmi-pvi.o
>  obj-$(CONFIG_DRM_IMX8QM_LDB) += imx8qm-ldb.o
>  obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
> diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> new file mode 100644
> index 000000000000..9002974073ca
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2025 NXP
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/component.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <drm/bridge/dw_hdmi.h>
> +#include <sound/asoundef.h>
> +
> +#define HTX_PAI_CTRL			0x00
> +#define   ENABLE			BIT(0)
> +
> +#define HTX_PAI_CTRL_EXT		0x04
> +#define   WTMK_HIGH_MASK		GENMASK(31, 24)
> +#define   WTMK_LOW_MASK			GENMASK(23, 16)
> +#define   NUM_CH_MASK			GENMASK(10, 8)
> +#define   WTMK_HIGH(n)			FIELD_PREP(WTMK_HIGH_MASK, (n))
> +#define   WTMK_LOW(n)			FIELD_PREP(WTMK_LOW_MASK, (n))
> +
> +#define HTX_PAI_FIELD_CTRL		0x08
> +#define   B_FILT			BIT(31)
> +#define   PARITY_EN			BIT(30)
> +#define   END_SEL			BIT(29)
> +#define   PRE_SEL			GENMASK(28, 24)
> +#define   D_SEL				GENMASK(23, 20)
> +#define   V_SEL				GENMASK(19, 15)
> +#define   U_SEL				GENMASK(14, 10)
> +#define   C_SEL				GENMASK(9, 5)
> +#define   P_SEL				GENMASK(4, 0)
> +
> +#define HTX_PAI_STAT			0x0c
> +#define HTX_PAI_IRQ_NOMASK		0x10
> +#define HTX_PAI_IRQ_MASKED		0x14
> +#define HTX_PAI_IRQ_MASK		0x18
> +
> +struct imx8mp_hdmi_pai {
> +	struct device	*dev;
> +	struct regmap	*regmap;
> +};
> +
> +static void imx8mp_hdmi_pai_enable(struct dw_hdmi *dw_hdmi, int channel,
> +				   int width, int rate, int non_pcm,
> +				   int iec958)
> +{
> +	const struct dw_hdmi_plat_data *pdata = dw_hdmi_to_plat_data(dw_hdmi);
> +	struct imx8mp_hdmi_pai *hdmi_pai = (struct imx8mp_hdmi_pai *)pdata->priv_audio;
> +	int val;
> +
> +	/* PAI set control extended */
> +	val =  WTMK_HIGH(3) | WTMK_LOW(3);
> +	val |= FIELD_PREP(NUM_CH_MASK, channel - 1);
> +	regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL_EXT, val);
> +
> +	/* IEC60958 format */
> +	if (iec958) {
> +		val = FIELD_PREP_CONST(P_SEL,
> +				       __bf_shf(IEC958_SUBFRAME_PARITY));
> +		val |= FIELD_PREP_CONST(C_SEL,
> +					__bf_shf(IEC958_SUBFRAME_CHANNEL_STATUS));
> +		val |= FIELD_PREP_CONST(U_SEL,
> +					__bf_shf(IEC958_SUBFRAME_USER_DATA));
> +		val |= FIELD_PREP_CONST(V_SEL,
> +					__bf_shf(IEC958_SUBFRAME_VALIDITY));
> +		val |= FIELD_PREP_CONST(D_SEL,
> +					__bf_shf(IEC958_SUBFRAME_SAMPLE_24_MASK));
> +		val |= FIELD_PREP_CONST(PRE_SEL,
> +					__bf_shf(IEC958_SUBFRAME_PREAMBLE_MASK));
> +	} else {
> +		/* PCM choose 24bit*/
> +		val = FIELD_PREP(D_SEL, width - 24);
> +	}
> +
> +	regmap_write(hdmi_pai->regmap, HTX_PAI_FIELD_CTRL, val);
> +
> +	/* PAI start running */
> +	regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL, ENABLE);
> +}
> +
> +static void imx8mp_hdmi_pai_disable(struct dw_hdmi *dw_hdmi)
> +{
> +	const struct dw_hdmi_plat_data *pdata = dw_hdmi_to_plat_data(dw_hdmi);
> +	struct imx8mp_hdmi_pai *hdmi_pai = (struct imx8mp_hdmi_pai *)pdata->priv_audio;
> +
> +	/* Stop PAI */
> +	regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL, 0);
> +}
> +
> +static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
> +{
> +	struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> +	struct imx8mp_hdmi_pai *hdmi_pai;
> +
> +	hdmi_pai = dev_get_drvdata(dev);
> +
> +	plat_data->enable_audio = imx8mp_hdmi_pai_enable;
> +	plat_data->disable_audio = imx8mp_hdmi_pai_disable;
> +	plat_data->priv_audio = hdmi_pai;
> +
> +	return 0;
> +}
> +
> +static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
> +{
> +	struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> +
> +	plat_data->enable_audio = NULL;
> +	plat_data->disable_audio = NULL;
> +	plat_data->priv_audio = NULL;
> +}
> +
> +static const struct component_ops imx8mp_hdmi_pai_ops = {
> +	.bind   = imx8mp_hdmi_pai_bind,
> +	.unbind = imx8mp_hdmi_pai_unbind,
> +};
> +
> +static bool imx8mp_hdmi_pai_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case HTX_PAI_IRQ_NOMASK:
> +	case HTX_PAI_IRQ_MASKED:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool imx8mp_hdmi_pai_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case HTX_PAI_CTRL:
> +	case HTX_PAI_CTRL_EXT:
> +	case HTX_PAI_FIELD_CTRL:
> +	case HTX_PAI_IRQ_NOMASK:
> +	case HTX_PAI_IRQ_MASKED:
> +	case HTX_PAI_IRQ_MASK:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config imx8mp_hdmi_pai_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.max_register = HTX_PAI_IRQ_MASK,
> +	.volatile_reg = imx8mp_hdmi_pai_volatile_reg,
> +	.writeable_reg = imx8mp_hdmi_pai_writeable_reg,
> +};
> +
> +static int imx8mp_hdmi_pai_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct imx8mp_hdmi_pai *hdmi_pai;
> +	struct resource *res;
> +	void __iomem *base;
> +
> +	hdmi_pai = devm_kzalloc(dev, sizeof(*hdmi_pai), GFP_KERNEL);
> +	if (!hdmi_pai)
> +		return -ENOMEM;
> +
> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	hdmi_pai->dev = dev;
> +
> +	hdmi_pai->regmap = devm_regmap_init_mmio(dev, base, &imx8mp_hdmi_pai_regmap_config);
> +	if (IS_ERR(hdmi_pai->regmap)) {
> +		dev_err(dev, "regmap init failed\n");
> +		return PTR_ERR(hdmi_pai->regmap);
> +	}
> +
> +	dev_set_drvdata(dev, hdmi_pai);
> +
> +	return component_add(dev, &imx8mp_hdmi_pai_ops);
> +}
> +
> +static void imx8mp_hdmi_pai_remove(struct platform_device *pdev)
> +{
> +	component_del(&pdev->dev, &imx8mp_hdmi_pai_ops);
> +}
> +
> +static const struct of_device_id imx8mp_hdmi_pai_of_table[] = {
> +	{ .compatible = "fsl,imx8mp-hdmi-pai" },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx8mp_hdmi_pai_of_table);
> +
> +static struct platform_driver imx8mp_hdmi_pai_platform_driver = {
> +	.probe		= imx8mp_hdmi_pai_probe,
> +	.remove		= imx8mp_hdmi_pai_remove,
> +	.driver		= {
> +		.name	= "imx8mp-hdmi-pai",
> +		.of_match_table = imx8mp_hdmi_pai_of_table,
> +	},
> +};
> +module_platform_driver(imx8mp_hdmi_pai_platform_driver);
> +
> +MODULE_DESCRIPTION("i.MX8MP HDMI PAI driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> index 1e7a789ec289..ee08084d2394 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> @@ -5,11 +5,13 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/component.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <drm/bridge/dw_hdmi.h>
>  #include <drm/drm_modes.h>
> +#include <drm/drm_of.h>
>  
>  struct imx8mp_hdmi {
>  	struct dw_hdmi_plat_data plat_data;
> @@ -79,11 +81,46 @@ static const struct dw_hdmi_phy_ops imx8mp_hdmi_phy_ops = {
>  	.update_hpd	= dw_hdmi_phy_update_hpd,
>  };
>  
> +static int imx8mp_dw_hdmi_bind(struct device *dev)
> +{
> +	struct dw_hdmi_plat_data *plat_data;
> +	struct imx8mp_hdmi *hdmi;
> +	int ret;
> +
> +	hdmi = dev_get_drvdata(dev);
> +	plat_data = &hdmi->plat_data;
> +
> +	ret = component_bind_all(dev, plat_data);

Do you really need plat_data variable?

> +	if (ret)
> +		return dev_err_probe(dev, ret, "component_bind_all failed!\n");
> +
> +	return 0;
> +}
> +
> +static void imx8mp_dw_hdmi_unbind(struct device *dev)
> +{
> +	struct dw_hdmi_plat_data *plat_data;
> +	struct imx8mp_hdmi *hdmi;
> +
> +	hdmi = dev_get_drvdata(dev);
> +	plat_data = &hdmi->plat_data;
> +
> +	component_unbind_all(dev, plat_data);

Do you really need plat_data variable?

> +}
> +
> +static const struct component_master_ops imx8mp_dw_hdmi_ops = {
> +	.bind   = imx8mp_dw_hdmi_bind,
> +	.unbind = imx8mp_dw_hdmi_unbind,
> +};
> +
>  static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct dw_hdmi_plat_data *plat_data;
> +	struct component_match *match;

Set match = NULL for drm_of_component_match_add (and subcalls) to allocate memory.

Best regards
Alexander

> +	struct device_node *remote;
>  	struct imx8mp_hdmi *hdmi;
> +	int ret;
>  
>  	hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
>  	if (!hdmi)
> @@ -108,6 +145,22 @@ static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, hdmi);
>  
> +	/* port@2 is for hdmi_pai device */
> +	remote = of_graph_get_remote_node(pdev->dev.of_node, 2, 0);
> +	if (remote && of_device_is_available(remote)) {
> +		drm_of_component_match_add(dev, &match, component_compare_of, remote);
> +
> +		of_node_put(remote);
> +
> +		ret = component_master_add_with_match(dev, &imx8mp_dw_hdmi_ops, match);
> +		if (ret)
> +			dev_warn(dev, "Unable to register aggregate driver\n");
> +		/*
> +		 * This audio function is optional for avoid blocking display.
> +		 * So just print warning message and no error is returned.
> +		 */
> +	}
> +
>  	return 0;
>  }
>  
> @@ -115,6 +168,8 @@ static void imx8mp_dw_hdmi_remove(struct platform_device *pdev)
>  {
>  	struct imx8mp_hdmi *hdmi = platform_get_drvdata(pdev);
>  
> +	component_master_del(&pdev->dev, &imx8mp_dw_hdmi_ops);
> +
>  	dw_hdmi_remove(hdmi->dw_hdmi);
>  }
>  
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 095cdd9b7424..336f062e1f9d 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -143,6 +143,12 @@ struct dw_hdmi_plat_data {
>  					   const struct drm_display_info *info,
>  					   const struct drm_display_mode *mode);
>  
> +	/*
> +	 * priv_audio is specially used for additional audio device to get
> +	 * driver data through this dw_hdmi_plat_data.
> +	 */
> +	void *priv_audio;
> +
>  	/* Platform-specific audio enable/disable (optional) */
>  	void (*enable_audio)(struct dw_hdmi *hdmi, int channel,
>  			     int width, int rate, int non_pcm, int iec958);
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v3 6/6] arm64: dts: imx8mp: Add hdmi parallel audio interface node

[Alexander Stein]

Am Montag, 4. August 2025, 12:47:22 CEST schrieb Shengjiu Wang:
> The HDMI TX Parallel Audio Interface (HTX_PAI) is a bridge between the
> Audio Subsystem to the HDMI TX Controller.
> 
> Shrink register map size of hdmi_pvi to avoid overlapped hdmi_pai device.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mp-evk.dts |  4 +++
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi    | 28 +++++++++++++++++++-

Please separate commits for SoC and board files. Thanks

Best regards,
Alexander

>  2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> index c0cc5611048e..cc9351a5bd65 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> @@ -442,6 +442,10 @@ &flexcan2 {
>  	status = "disabled";/* can2 pin conflict with pdm */
>  };
>  
> +&hdmi_pai {
> +	status = "okay";
> +};
> +
>  &hdmi_pvi {
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index 841d155685ee..00d8474bd1b1 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -2066,7 +2066,7 @@ irqsteer_hdmi: interrupt-controller@32fc2000 {
>  
>  			hdmi_pvi: display-bridge@32fc4000 {
>  				compatible = "fsl,imx8mp-hdmi-pvi";
> -				reg = <0x32fc4000 0x1000>;
> +				reg = <0x32fc4000 0x800>;
>  				interrupt-parent = <&irqsteer_hdmi>;
>  				interrupts = <12>;
>  				power-domains = <&hdmi_blk_ctrl IMX8MP_HDMIBLK_PD_PVI>;
> @@ -2092,6 +2092,24 @@ pvi_to_hdmi_tx: endpoint {
>  				};
>  			};
>  
> +			hdmi_pai: audio-bridge@32fc4800 {
> +				compatible = "fsl,imx8mp-hdmi-pai";
> +				reg = <0x32fc4800 0x800>;
> +				interrupt-parent = <&irqsteer_hdmi>;
> +				interrupts = <14>;
> +				clocks = <&clk IMX8MP_CLK_HDMI_APB>;
> +				clock-names = "apb";
> +				power-domains = <&hdmi_blk_ctrl IMX8MP_HDMIBLK_PD_PAI>;
> +				status = "disabled";
> +
> +				port {
> +
> +					pai_to_hdmi_tx: endpoint {
> +						remote-endpoint = <&hdmi_tx_from_pai>;
> +					};
> +				};
> +			};
> +
>  			lcdif3: display-controller@32fc6000 {
>  				compatible = "fsl,imx8mp-lcdif";
>  				reg = <0x32fc6000 0x1000>;
> @@ -2143,6 +2161,14 @@ port@1 {
>  						reg = <1>;
>  						/* Point endpoint to the HDMI connector */
>  					};
> +
> +					port@2 {
> +						reg = <2>;
> +
> +						hdmi_tx_from_pai: endpoint {
> +							remote-endpoint = <&pai_to_hdmi_tx>;
> +						};
> +					};
>  				};
>  			};
>  
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Liu Ying]

On 08/04/2025, Shengjiu Wang wrote:
> The HDMI TX Parallel Audio Interface (HTX_PAI) is a digital module that
> acts as the bridge between the Audio Subsystem to the HDMI TX Controller.
> This IP block is found in the HDMI subsystem of the i.MX8MP SoC.
> 
> Data received from the audio subsystem can have an arbitrary component
> ordering. The HTX_PAI block has integrated muxing options to select which
> sections of the 32-bit input data word will be mapped to each IEC60958
> field. The HTX_PAI_FIELD_CTRL register contains mux selects to
> individually select P,C,U,V,Data, and Preamble.
> 
> Use component helper that imx8mp-hdmi-tx will be aggregate driver,

s/that/so that/

> imx8mp-hdmi-pai will be component driver, then imx8mp-hdmi-pai can use
> bind() ops to get the plat_data from imx8mp-hdmi-tx device.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  drivers/gpu/drm/bridge/imx/Kconfig           |   8 +
>  drivers/gpu/drm/bridge/imx/Makefile          |   1 +
>  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c | 205 +++++++++++++++++++
>  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c  |  55 +++++
>  include/drm/bridge/dw_hdmi.h                 |   6 +
>  5 files changed, 275 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> 
> diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig
> index 9a480c6abb85..6c1a8bc5d4a0 100644
> --- a/drivers/gpu/drm/bridge/imx/Kconfig
> +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> @@ -18,12 +18,20 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE
>  	depends on OF
>  	depends on COMMON_CLK
>  	select DRM_DW_HDMI
> +	imply DRM_IMX8MP_HDMI_PAI
>  	imply DRM_IMX8MP_HDMI_PVI
>  	imply PHY_FSL_SAMSUNG_HDMI_PHY
>  	help
>  	  Choose this to enable support for the internal HDMI encoder found
>  	  on the i.MX8MP SoC.
>  
> +config DRM_IMX8MP_HDMI_PAI
> +	tristate "Freescale i.MX8MP HDMI PAI bridge support"
> +	depends on OF

select REGMAP
select REGMAP_MMIO

> +	help
> +	  Choose this to enable support for the internal HDMI TX Parallel
> +	  Audio Interface found on the Freescale i.MX8MP SoC.
> +
>  config DRM_IMX8MP_HDMI_PVI
>  	tristate "Freescale i.MX8MP HDMI PVI bridge support"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile
> index dd5d48584806..8d01fda25451 100644
> --- a/drivers/gpu/drm/bridge/imx/Makefile
> +++ b/drivers/gpu/drm/bridge/imx/Makefile
> @@ -1,6 +1,7 @@
>  obj-$(CONFIG_DRM_IMX_LDB_HELPER) += imx-ldb-helper.o
>  obj-$(CONFIG_DRM_IMX_LEGACY_BRIDGE) += imx-legacy-bridge.o
>  obj-$(CONFIG_DRM_IMX8MP_DW_HDMI_BRIDGE) += imx8mp-hdmi-tx.o
> +obj-$(CONFIG_DRM_IMX8MP_HDMI_PAI) += imx8mp-hdmi-pai.o
>  obj-$(CONFIG_DRM_IMX8MP_HDMI_PVI) += imx8mp-hdmi-pvi.o
>  obj-$(CONFIG_DRM_IMX8QM_LDB) += imx8qm-ldb.o
>  obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
> diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> new file mode 100644
> index 000000000000..9002974073ca
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2025 NXP
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/component.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>

#include <linux/regmap.h>

> +#include <drm/bridge/dw_hdmi.h>
> +#include <sound/asoundef.h>
> +
> +#define HTX_PAI_CTRL			0x00
> +#define   ENABLE			BIT(0)
> +
> +#define HTX_PAI_CTRL_EXT		0x04
> +#define   WTMK_HIGH_MASK		GENMASK(31, 24)
> +#define   WTMK_LOW_MASK			GENMASK(23, 16)
> +#define   NUM_CH_MASK			GENMASK(10, 8)

Add NUM_CH(n) and use it when programming HTX_PAI_CTRL_EXT.
#define NUM_CH(n)			FIELD_PREP(NUM_CH_MASK, (n - 1))

> +#define   WTMK_HIGH(n)			FIELD_PREP(WTMK_HIGH_MASK, (n))
> +#define   WTMK_LOW(n)			FIELD_PREP(WTMK_LOW_MASK, (n))
> +
> +#define HTX_PAI_FIELD_CTRL		0x08
> +#define   B_FILT			BIT(31)
> +#define   PARITY_EN			BIT(30)
> +#define   END_SEL			BIT(29)

The above 3 bits are unused.  Drop.

> +#define   PRE_SEL			GENMASK(28, 24)
> +#define   D_SEL				GENMASK(23, 20)
> +#define   V_SEL				GENMASK(19, 15)
> +#define   U_SEL				GENMASK(14, 10)
> +#define   C_SEL				GENMASK(9, 5)
> +#define   P_SEL				GENMASK(4, 0)
> +
> +#define HTX_PAI_STAT			0x0c
> +#define HTX_PAI_IRQ_NOMASK		0x10
> +#define HTX_PAI_IRQ_MASKED		0x14
> +#define HTX_PAI_IRQ_MASK		0x18

The above 4 registers are not pratically used.  Drop.

> +
> +struct imx8mp_hdmi_pai {
> +	struct device	*dev;
> +	struct regmap	*regmap;
> +};
> +
> +static void imx8mp_hdmi_pai_enable(struct dw_hdmi *dw_hdmi, int channel,
> +				   int width, int rate, int non_pcm,
> +				   int iec958)
> +{
> +	const struct dw_hdmi_plat_data *pdata = dw_hdmi_to_plat_data(dw_hdmi);
> +	struct imx8mp_hdmi_pai *hdmi_pai = (struct imx8mp_hdmi_pai *)pdata->priv_audio;

I don't think you need to convert type explicitly.  Same for the other
explicit conversions in this driver.

> +	int val;
> +
> +	/* PAI set control extended */
> +	val =  WTMK_HIGH(3) | WTMK_LOW(3);
> +	val |= FIELD_PREP(NUM_CH_MASK, channel - 1);
> +	regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL_EXT, val);
> +
> +	/* IEC60958 format */
> +	if (iec958) {
> +		val = FIELD_PREP_CONST(P_SEL,
> +				       __bf_shf(IEC958_SUBFRAME_PARITY));
> +		val |= FIELD_PREP_CONST(C_SEL,
> +					__bf_shf(IEC958_SUBFRAME_CHANNEL_STATUS));
> +		val |= FIELD_PREP_CONST(U_SEL,
> +					__bf_shf(IEC958_SUBFRAME_USER_DATA));
> +		val |= FIELD_PREP_CONST(V_SEL,
> +					__bf_shf(IEC958_SUBFRAME_VALIDITY));
> +		val |= FIELD_PREP_CONST(D_SEL,
> +					__bf_shf(IEC958_SUBFRAME_SAMPLE_24_MASK));
> +		val |= FIELD_PREP_CONST(PRE_SEL,
> +					__bf_shf(IEC958_SUBFRAME_PREAMBLE_MASK));
> +	} else {
> +		/* PCM choose 24bit*/
> +		val = FIELD_PREP(D_SEL, width - 24);

Why 'width - 24'?  Can it be expressed by a helper or macro?

> +	}
> +
> +	regmap_write(hdmi_pai->regmap, HTX_PAI_FIELD_CTRL, val);
> +
> +	/* PAI start running */
> +	regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL, ENABLE);
> +}
> +
> +static void imx8mp_hdmi_pai_disable(struct dw_hdmi *dw_hdmi)
> +{
> +	const struct dw_hdmi_plat_data *pdata = dw_hdmi_to_plat_data(dw_hdmi);
> +	struct imx8mp_hdmi_pai *hdmi_pai = (struct imx8mp_hdmi_pai *)pdata->priv_audio;
> +
> +	/* Stop PAI */
> +	regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL, 0);
> +}
> +
> +static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
> +{
> +	struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> +	struct imx8mp_hdmi_pai *hdmi_pai;
> +
> +	hdmi_pai = dev_get_drvdata(dev);
> +
> +	plat_data->enable_audio = imx8mp_hdmi_pai_enable;
> +	plat_data->disable_audio = imx8mp_hdmi_pai_disable;
> +	plat_data->priv_audio = hdmi_pai;
> +
> +	return 0;
> +}
> +
> +static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
> +{
> +	struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> +
> +	plat_data->enable_audio = NULL;
> +	plat_data->disable_audio = NULL;
> +	plat_data->priv_audio = NULL;

Do you really need to set these ptrs to NULL?

> +}
> +
> +static const struct component_ops imx8mp_hdmi_pai_ops = {
> +	.bind   = imx8mp_hdmi_pai_bind,
> +	.unbind = imx8mp_hdmi_pai_unbind,
> +};
> +
> +static bool imx8mp_hdmi_pai_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case HTX_PAI_IRQ_NOMASK:
> +	case HTX_PAI_IRQ_MASKED:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool imx8mp_hdmi_pai_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case HTX_PAI_CTRL:
> +	case HTX_PAI_CTRL_EXT:
> +	case HTX_PAI_FIELD_CTRL:
> +	case HTX_PAI_IRQ_NOMASK:
> +	case HTX_PAI_IRQ_MASKED:
> +	case HTX_PAI_IRQ_MASK:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config imx8mp_hdmi_pai_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.max_register = HTX_PAI_IRQ_MASK,
> +	.volatile_reg = imx8mp_hdmi_pai_volatile_reg,
> +	.writeable_reg = imx8mp_hdmi_pai_writeable_reg,
> +};
> +
> +static int imx8mp_hdmi_pai_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct imx8mp_hdmi_pai *hdmi_pai;
> +	struct resource *res;
> +	void __iomem *base;
> +
> +	hdmi_pai = devm_kzalloc(dev, sizeof(*hdmi_pai), GFP_KERNEL);
> +	if (!hdmi_pai)
> +		return -ENOMEM;
> +
> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	hdmi_pai->dev = dev;
> +
> +	hdmi_pai->regmap = devm_regmap_init_mmio(dev, base, &imx8mp_hdmi_pai_regmap_config);

Now that DT binding says there is an APB clock, use devm_regmap_init_mmio_clk()
to ensure registers can be accessed with clock enabled even via debugfs.

> +	if (IS_ERR(hdmi_pai->regmap)) {
> +		dev_err(dev, "regmap init failed\n");
> +		return PTR_ERR(hdmi_pai->regmap);
> +	}
> +
> +	dev_set_drvdata(dev, hdmi_pai);
> +
> +	return component_add(dev, &imx8mp_hdmi_pai_ops);

Imagine that users could enable this driver without enabling imx8mp-hdmi-tx
driver, you may add the component in this probe() callback only and move all
the other stuff to bind() callback to avoid unnecessary things being done here.

> +}
> +
> +static void imx8mp_hdmi_pai_remove(struct platform_device *pdev)
> +{
> +	component_del(&pdev->dev, &imx8mp_hdmi_pai_ops);
> +}
> +
> +static const struct of_device_id imx8mp_hdmi_pai_of_table[] = {
> +	{ .compatible = "fsl,imx8mp-hdmi-pai" },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx8mp_hdmi_pai_of_table);
> +
> +static struct platform_driver imx8mp_hdmi_pai_platform_driver = {
> +	.probe		= imx8mp_hdmi_pai_probe,
> +	.remove		= imx8mp_hdmi_pai_remove,
> +	.driver		= {
> +		.name	= "imx8mp-hdmi-pai",
> +		.of_match_table = imx8mp_hdmi_pai_of_table,
> +	},
> +};
> +module_platform_driver(imx8mp_hdmi_pai_platform_driver);
> +
> +MODULE_DESCRIPTION("i.MX8MP HDMI PAI driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> index 1e7a789ec289..ee08084d2394 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> @@ -5,11 +5,13 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/component.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <drm/bridge/dw_hdmi.h>
>  #include <drm/drm_modes.h>
> +#include <drm/drm_of.h>
>  
>  struct imx8mp_hdmi {
>  	struct dw_hdmi_plat_data plat_data;
> @@ -79,11 +81,46 @@ static const struct dw_hdmi_phy_ops imx8mp_hdmi_phy_ops = {
>  	.update_hpd	= dw_hdmi_phy_update_hpd,
>  };
>  
> +static int imx8mp_dw_hdmi_bind(struct device *dev)
> +{
> +	struct dw_hdmi_plat_data *plat_data;
> +	struct imx8mp_hdmi *hdmi;
> +	int ret;
> +
> +	hdmi = dev_get_drvdata(dev);
> +	plat_data = &hdmi->plat_data;
> +
> +	ret = component_bind_all(dev, plat_data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "component_bind_all failed!\n");

As component_bind_all() would bind imx8mp-hdmi-pai and hence set
{enable,disable}_audio callbacks, you need to call dw_hdmi_probe() after
component_bind_all() instead of too early in probe() callback.

> +
> +	return 0;
> +}
> +
> +static void imx8mp_dw_hdmi_unbind(struct device *dev)
> +{
> +	struct dw_hdmi_plat_data *plat_data;
> +	struct imx8mp_hdmi *hdmi;
> +
> +	hdmi = dev_get_drvdata(dev);
> +	plat_data = &hdmi->plat_data;
> +
> +	component_unbind_all(dev, plat_data);
> +}
> +
> +static const struct component_master_ops imx8mp_dw_hdmi_ops = {
> +	.bind   = imx8mp_dw_hdmi_bind,
> +	.unbind = imx8mp_dw_hdmi_unbind,
> +};
> +
>  static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct dw_hdmi_plat_data *plat_data;
> +	struct component_match *match;

As Alexander pointed out, this needs to be set to NULL.
See how other drivers which are component masters do.

> +	struct device_node *remote;
>  	struct imx8mp_hdmi *hdmi;
> +	int ret;
>  
>  	hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
>  	if (!hdmi)
> @@ -108,6 +145,22 @@ static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, hdmi);
>  
> +	/* port@2 is for hdmi_pai device */
> +	remote = of_graph_get_remote_node(pdev->dev.of_node, 2, 0);
> +	if (remote && of_device_is_available(remote)) {

Doesn't of_graph_get_remote_node() ensure that remote is avaiable?

> +		drm_of_component_match_add(dev, &match, component_compare_of, remote);
> +
> +		of_node_put(remote);
> +
> +		ret = component_master_add_with_match(dev, &imx8mp_dw_hdmi_ops, match);
> +		if (ret)
> +			dev_warn(dev, "Unable to register aggregate driver\n");
> +		/*
> +		 * This audio function is optional for avoid blocking display.
> +		 * So just print warning message and no error is returned.

No, since PAI node is available here, it has to be bound.  Yet you still need
to properly handle the case where PAI node is inavailable.

> +		 */
> +	}
> +
>  	return 0;
>  }
>  
> @@ -115,6 +168,8 @@ static void imx8mp_dw_hdmi_remove(struct platform_device *pdev)
>  {
>  	struct imx8mp_hdmi *hdmi = platform_get_drvdata(pdev);
>  
> +	component_master_del(&pdev->dev, &imx8mp_dw_hdmi_ops);
> +
>  	dw_hdmi_remove(hdmi->dw_hdmi);
>  }
>  
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 095cdd9b7424..336f062e1f9d 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -143,6 +143,12 @@ struct dw_hdmi_plat_data {
>  					   const struct drm_display_info *info,
>  					   const struct drm_display_mode *mode);
>  
> +	/*
> +	 * priv_audio is specially used for additional audio device to get
> +	 * driver data through this dw_hdmi_plat_data.
> +	 */
> +	void *priv_audio;
> +
>  	/* Platform-specific audio enable/disable (optional) */
>  	void (*enable_audio)(struct dw_hdmi *hdmi, int channel,
>  			     int width, int rate, int non_pcm, int iec958);


-- 
Regards,
Liu Ying

Re: [PATCH v3 3/6] drm/bridge: dw-hdmi: Add API dw_hdmi_to_plat_data() to get plat_data

[Liu Ying]

On 08/04/2025, Shengjiu Wang wrote:
> Add API dw_hdmi_to_plat_data() to fetch plat_data because audio device
> driver needs it to enable(disable)_audio().
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 6 ++++++
>  include/drm/bridge/dw_hdmi.h              | 2 ++
>  2 files changed, 8 insertions(+)

Acked-by: Liu Ying <victor.liu@nxp.com>

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[kernel test robot]

Hi Shengjiu,

kernel test robot noticed the following build errors:

[auto build test ERROR on shawnguo/for-next]
[also build test ERROR on robh/for-next tiwai-sound/for-next tiwai-sound/for-linus linus/master v6.16 next-20250805]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shengjiu-Wang/dt-bindings-display-imx-add-HDMI-PAI-for-i-MX8MP/20250804-185254
base:   https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git for-next
patch link:    https://lore.kernel.org/r/20250804104722.601440-6-shengjiu.wang%40nxp.com
patch subject: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface
config: xtensa-randconfig-001-20250805 (https://download.01.org/0day-ci/archive/20250805/202508052225.u407ulE0-lkp@intel.com/config)
compiler: xtensa-linux-gcc (GCC) 14.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250805/202508052225.u407ulE0-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508052225.u407ulE0-lkp@intel.com/

All errors (new ones prefixed by >>):

   `.exit.text' referenced in section `__jump_table' of lib/test_dynamic_debug.o: defined in discarded section `.exit.text' of lib/test_dynamic_debug.o
   `.exit.text' referenced in section `__jump_table' of lib/test_dynamic_debug.o: defined in discarded section `.exit.text' of lib/test_dynamic_debug.o
   xtensa-linux-ld: drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.o: in function `imx8mp_hdmi_pai_remove':
>> imx8mp-hdmi-pai.c:(.text+0x8c): undefined reference to `dw_hdmi_to_plat_data'
   xtensa-linux-ld: drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.o: in function `imx8mp_hdmi_pai_disable':
   imx8mp-hdmi-pai.c:(.text+0x9a): undefined reference to `dw_hdmi_to_plat_data'
>> xtensa-linux-ld: imx8mp-hdmi-pai.c:(.text+0xbc): undefined reference to `dw_hdmi_to_plat_data'
   xtensa-linux-ld: drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.o: in function `imx8mp_hdmi_pai_enable':
   imx8mp-hdmi-pai.c:(.text+0xd2): undefined reference to `dw_hdmi_to_plat_data'
   `.exit.text' referenced in section `__jump_table' of net/ceph/ceph_common.o: defined in discarded section `.exit.text' of net/ceph/ceph_common.o
   `.exit.text' referenced in section `__jump_table' of net/ceph/ceph_common.o: defined in discarded section `.exit.text' of net/ceph/ceph_common.o

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Shengjiu Wang]

On Tue, Aug 5, 2025 at 3:09 PM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi,
>
> Am Montag, 4. August 2025, 12:47:21 CEST schrieb Shengjiu Wang:
> > The HDMI TX Parallel Audio Interface (HTX_PAI) is a digital module that
> > acts as the bridge between the Audio Subsystem to the HDMI TX Controller.
> > This IP block is found in the HDMI subsystem of the i.MX8MP SoC.
> >
> > Data received from the audio subsystem can have an arbitrary component
> > ordering. The HTX_PAI block has integrated muxing options to select which
> > sections of the 32-bit input data word will be mapped to each IEC60958
> > field. The HTX_PAI_FIELD_CTRL register contains mux selects to
> > individually select P,C,U,V,Data, and Preamble.
> >
> > Use component helper that imx8mp-hdmi-tx will be aggregate driver,
> > imx8mp-hdmi-pai will be component driver, then imx8mp-hdmi-pai can use
> > bind() ops to get the plat_data from imx8mp-hdmi-tx device.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  drivers/gpu/drm/bridge/imx/Kconfig           |   8 +
> >  drivers/gpu/drm/bridge/imx/Makefile          |   1 +
> >  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c | 205 +++++++++++++++++++
> >  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c  |  55 +++++
> >  include/drm/bridge/dw_hdmi.h                 |   6 +
> >  5 files changed, 275 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> >
> > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig
> > index 9a480c6abb85..6c1a8bc5d4a0 100644
> > --- a/drivers/gpu/drm/bridge/imx/Kconfig
> > +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> > @@ -18,12 +18,20 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE
> >       depends on OF
> >       depends on COMMON_CLK
> >       select DRM_DW_HDMI
> > +     imply DRM_IMX8MP_HDMI_PAI
> >       imply DRM_IMX8MP_HDMI_PVI
> >       imply PHY_FSL_SAMSUNG_HDMI_PHY
> >       help
> >         Choose this to enable support for the internal HDMI encoder found
> >         on the i.MX8MP SoC.
> >
> > +config DRM_IMX8MP_HDMI_PAI
> > +     tristate "Freescale i.MX8MP HDMI PAI bridge support"
> > +     depends on OF
> > +     help
> > +       Choose this to enable support for the internal HDMI TX Parallel
> > +       Audio Interface found on the Freescale i.MX8MP SoC.
> > +
> >  config DRM_IMX8MP_HDMI_PVI
> >       tristate "Freescale i.MX8MP HDMI PVI bridge support"
> >       depends on OF
> > diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile
> > index dd5d48584806..8d01fda25451 100644
> > --- a/drivers/gpu/drm/bridge/imx/Makefile
> > +++ b/drivers/gpu/drm/bridge/imx/Makefile
> > @@ -1,6 +1,7 @@
> >  obj-$(CONFIG_DRM_IMX_LDB_HELPER) += imx-ldb-helper.o
> >  obj-$(CONFIG_DRM_IMX_LEGACY_BRIDGE) += imx-legacy-bridge.o
> >  obj-$(CONFIG_DRM_IMX8MP_DW_HDMI_BRIDGE) += imx8mp-hdmi-tx.o
> > +obj-$(CONFIG_DRM_IMX8MP_HDMI_PAI) += imx8mp-hdmi-pai.o
> >  obj-$(CONFIG_DRM_IMX8MP_HDMI_PVI) += imx8mp-hdmi-pvi.o
> >  obj-$(CONFIG_DRM_IMX8QM_LDB) += imx8qm-ldb.o
> >  obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
> > diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> > new file mode 100644
> > index 000000000000..9002974073ca
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> > @@ -0,0 +1,205 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2025 NXP
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/component.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <drm/bridge/dw_hdmi.h>
> > +#include <sound/asoundef.h>
> > +
> > +#define HTX_PAI_CTRL                 0x00
> > +#define   ENABLE                     BIT(0)
> > +
> > +#define HTX_PAI_CTRL_EXT             0x04
> > +#define   WTMK_HIGH_MASK             GENMASK(31, 24)
> > +#define   WTMK_LOW_MASK                      GENMASK(23, 16)
> > +#define   NUM_CH_MASK                        GENMASK(10, 8)
> > +#define   WTMK_HIGH(n)                       FIELD_PREP(WTMK_HIGH_MASK, (n))
> > +#define   WTMK_LOW(n)                        FIELD_PREP(WTMK_LOW_MASK, (n))
> > +
> > +#define HTX_PAI_FIELD_CTRL           0x08
> > +#define   B_FILT                     BIT(31)
> > +#define   PARITY_EN                  BIT(30)
> > +#define   END_SEL                    BIT(29)
> > +#define   PRE_SEL                    GENMASK(28, 24)
> > +#define   D_SEL                              GENMASK(23, 20)
> > +#define   V_SEL                              GENMASK(19, 15)
> > +#define   U_SEL                              GENMASK(14, 10)
> > +#define   C_SEL                              GENMASK(9, 5)
> > +#define   P_SEL                              GENMASK(4, 0)
> > +
> > +#define HTX_PAI_STAT                 0x0c
> > +#define HTX_PAI_IRQ_NOMASK           0x10
> > +#define HTX_PAI_IRQ_MASKED           0x14
> > +#define HTX_PAI_IRQ_MASK             0x18
> > +
> > +struct imx8mp_hdmi_pai {
> > +     struct device   *dev;
> > +     struct regmap   *regmap;
> > +};
> > +
> > +static void imx8mp_hdmi_pai_enable(struct dw_hdmi *dw_hdmi, int channel,
> > +                                int width, int rate, int non_pcm,
> > +                                int iec958)
> > +{
> > +     const struct dw_hdmi_plat_data *pdata = dw_hdmi_to_plat_data(dw_hdmi);
> > +     struct imx8mp_hdmi_pai *hdmi_pai = (struct imx8mp_hdmi_pai *)pdata->priv_audio;
> > +     int val;
> > +
> > +     /* PAI set control extended */
> > +     val =  WTMK_HIGH(3) | WTMK_LOW(3);
> > +     val |= FIELD_PREP(NUM_CH_MASK, channel - 1);
> > +     regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL_EXT, val);
> > +
> > +     /* IEC60958 format */
> > +     if (iec958) {
> > +             val = FIELD_PREP_CONST(P_SEL,
> > +                                    __bf_shf(IEC958_SUBFRAME_PARITY));
> > +             val |= FIELD_PREP_CONST(C_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_CHANNEL_STATUS));
> > +             val |= FIELD_PREP_CONST(U_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_USER_DATA));
> > +             val |= FIELD_PREP_CONST(V_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_VALIDITY));
> > +             val |= FIELD_PREP_CONST(D_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_SAMPLE_24_MASK));
> > +             val |= FIELD_PREP_CONST(PRE_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_PREAMBLE_MASK));
> > +     } else {
> > +             /* PCM choose 24bit*/
> > +             val = FIELD_PREP(D_SEL, width - 24);
> > +     }
> > +
> > +     regmap_write(hdmi_pai->regmap, HTX_PAI_FIELD_CTRL, val);
> > +
> > +     /* PAI start running */
> > +     regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL, ENABLE);
> > +}
> > +
> > +static void imx8mp_hdmi_pai_disable(struct dw_hdmi *dw_hdmi)
> > +{
> > +     const struct dw_hdmi_plat_data *pdata = dw_hdmi_to_plat_data(dw_hdmi);
> > +     struct imx8mp_hdmi_pai *hdmi_pai = (struct imx8mp_hdmi_pai *)pdata->priv_audio;
> > +
> > +     /* Stop PAI */
> > +     regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL, 0);
> > +}
> > +
> > +static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
> > +{
> > +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> > +     struct imx8mp_hdmi_pai *hdmi_pai;
> > +
> > +     hdmi_pai = dev_get_drvdata(dev);
> > +
> > +     plat_data->enable_audio = imx8mp_hdmi_pai_enable;
> > +     plat_data->disable_audio = imx8mp_hdmi_pai_disable;
> > +     plat_data->priv_audio = hdmi_pai;
> > +
> > +     return 0;
> > +}
> > +
> > +static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
> > +{
> > +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> > +
> > +     plat_data->enable_audio = NULL;
> > +     plat_data->disable_audio = NULL;
> > +     plat_data->priv_audio = NULL;
> > +}
> > +
> > +static const struct component_ops imx8mp_hdmi_pai_ops = {
> > +     .bind   = imx8mp_hdmi_pai_bind,
> > +     .unbind = imx8mp_hdmi_pai_unbind,
> > +};
> > +
> > +static bool imx8mp_hdmi_pai_volatile_reg(struct device *dev, unsigned int reg)
> > +{
> > +     switch (reg) {
> > +     case HTX_PAI_IRQ_NOMASK:
> > +     case HTX_PAI_IRQ_MASKED:
> > +             return true;
> > +     default:
> > +             return false;
> > +     }
> > +}
> > +
> > +static bool imx8mp_hdmi_pai_writeable_reg(struct device *dev, unsigned int reg)
> > +{
> > +     switch (reg) {
> > +     case HTX_PAI_CTRL:
> > +     case HTX_PAI_CTRL_EXT:
> > +     case HTX_PAI_FIELD_CTRL:
> > +     case HTX_PAI_IRQ_NOMASK:
> > +     case HTX_PAI_IRQ_MASKED:
> > +     case HTX_PAI_IRQ_MASK:
> > +             return true;
> > +     default:
> > +             return false;
> > +     }
> > +}
> > +
> > +static const struct regmap_config imx8mp_hdmi_pai_regmap_config = {
> > +     .reg_bits = 32,
> > +     .reg_stride = 4,
> > +     .val_bits = 32,
> > +     .max_register = HTX_PAI_IRQ_MASK,
> > +     .volatile_reg = imx8mp_hdmi_pai_volatile_reg,
> > +     .writeable_reg = imx8mp_hdmi_pai_writeable_reg,
> > +};
> > +
> > +static int imx8mp_hdmi_pai_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct imx8mp_hdmi_pai *hdmi_pai;
> > +     struct resource *res;
> > +     void __iomem *base;
> > +
> > +     hdmi_pai = devm_kzalloc(dev, sizeof(*hdmi_pai), GFP_KERNEL);
> > +     if (!hdmi_pai)
> > +             return -ENOMEM;
> > +
> > +     base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     hdmi_pai->dev = dev;
> > +
> > +     hdmi_pai->regmap = devm_regmap_init_mmio(dev, base, &imx8mp_hdmi_pai_regmap_config);
> > +     if (IS_ERR(hdmi_pai->regmap)) {
> > +             dev_err(dev, "regmap init failed\n");
> > +             return PTR_ERR(hdmi_pai->regmap);
> > +     }
> > +
> > +     dev_set_drvdata(dev, hdmi_pai);
> > +
> > +     return component_add(dev, &imx8mp_hdmi_pai_ops);
> > +}
> > +
> > +static void imx8mp_hdmi_pai_remove(struct platform_device *pdev)
> > +{
> > +     component_del(&pdev->dev, &imx8mp_hdmi_pai_ops);
> > +}
> > +
> > +static const struct of_device_id imx8mp_hdmi_pai_of_table[] = {
> > +     { .compatible = "fsl,imx8mp-hdmi-pai" },
> > +     { /* Sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx8mp_hdmi_pai_of_table);
> > +
> > +static struct platform_driver imx8mp_hdmi_pai_platform_driver = {
> > +     .probe          = imx8mp_hdmi_pai_probe,
> > +     .remove         = imx8mp_hdmi_pai_remove,
> > +     .driver         = {
> > +             .name   = "imx8mp-hdmi-pai",
> > +             .of_match_table = imx8mp_hdmi_pai_of_table,
> > +     },
> > +};
> > +module_platform_driver(imx8mp_hdmi_pai_platform_driver);
> > +
> > +MODULE_DESCRIPTION("i.MX8MP HDMI PAI driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> > index 1e7a789ec289..ee08084d2394 100644
> > --- a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> > +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> > @@ -5,11 +5,13 @@
> >   */
> >
> >  #include <linux/clk.h>
> > +#include <linux/component.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> >  #include <drm/bridge/dw_hdmi.h>
> >  #include <drm/drm_modes.h>
> > +#include <drm/drm_of.h>
> >
> >  struct imx8mp_hdmi {
> >       struct dw_hdmi_plat_data plat_data;
> > @@ -79,11 +81,46 @@ static const struct dw_hdmi_phy_ops imx8mp_hdmi_phy_ops = {
> >       .update_hpd     = dw_hdmi_phy_update_hpd,
> >  };
> >
> > +static int imx8mp_dw_hdmi_bind(struct device *dev)
> > +{
> > +     struct dw_hdmi_plat_data *plat_data;
> > +     struct imx8mp_hdmi *hdmi;
> > +     int ret;
> > +
> > +     hdmi = dev_get_drvdata(dev);
> > +     plat_data = &hdmi->plat_data;
> > +
> > +     ret = component_bind_all(dev, plat_data);
>
> Do you really need plat_data variable?

yes,  it is used in imx8mp_hdmi_pai_bind()

>
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "component_bind_all failed!\n");
> > +
> > +     return 0;
> > +}
> > +
> > +static void imx8mp_dw_hdmi_unbind(struct device *dev)
> > +{
> > +     struct dw_hdmi_plat_data *plat_data;
> > +     struct imx8mp_hdmi *hdmi;
> > +
> > +     hdmi = dev_get_drvdata(dev);
> > +     plat_data = &hdmi->plat_data;
> > +
> > +     component_unbind_all(dev, plat_data);
>
> Do you really need plat_data variable?

yes,  it is used by imx8mp_hdmi_pai_unbind()

>
> > +}
> > +
> > +static const struct component_master_ops imx8mp_dw_hdmi_ops = {
> > +     .bind   = imx8mp_dw_hdmi_bind,
> > +     .unbind = imx8mp_dw_hdmi_unbind,
> > +};
> > +
> >  static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
> >  {
> >       struct device *dev = &pdev->dev;
> >       struct dw_hdmi_plat_data *plat_data;
> > +     struct component_match *match;
>
> Set match = NULL for drm_of_component_match_add (and subcalls) to allocate memory.

Ok.

best regards
Shengjiu wang.
>
> Best regards
> Alexander
>
> > +     struct device_node *remote;
> >       struct imx8mp_hdmi *hdmi;
> > +     int ret;
> >
> >       hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
> >       if (!hdmi)
> > @@ -108,6 +145,22 @@ static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
> >
> >       platform_set_drvdata(pdev, hdmi);
> >
> > +     /* port@2 is for hdmi_pai device */
> > +     remote = of_graph_get_remote_node(pdev->dev.of_node, 2, 0);
> > +     if (remote && of_device_is_available(remote)) {
> > +             drm_of_component_match_add(dev, &match, component_compare_of, remote);
> > +
> > +             of_node_put(remote);
> > +
> > +             ret = component_master_add_with_match(dev, &imx8mp_dw_hdmi_ops, match);
> > +             if (ret)
> > +                     dev_warn(dev, "Unable to register aggregate driver\n");
> > +             /*
> > +              * This audio function is optional for avoid blocking display.
> > +              * So just print warning message and no error is returned.
> > +              */
> > +     }
> > +
> >       return 0;
> >  }
> >
> > @@ -115,6 +168,8 @@ static void imx8mp_dw_hdmi_remove(struct platform_device *pdev)
> >  {
> >       struct imx8mp_hdmi *hdmi = platform_get_drvdata(pdev);
> >
> > +     component_master_del(&pdev->dev, &imx8mp_dw_hdmi_ops);
> > +
> >       dw_hdmi_remove(hdmi->dw_hdmi);
> >  }
> >
> > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> > index 095cdd9b7424..336f062e1f9d 100644
> > --- a/include/drm/bridge/dw_hdmi.h
> > +++ b/include/drm/bridge/dw_hdmi.h
> > @@ -143,6 +143,12 @@ struct dw_hdmi_plat_data {
> >                                          const struct drm_display_info *info,
> >                                          const struct drm_display_mode *mode);
> >
> > +     /*
> > +      * priv_audio is specially used for additional audio device to get
> > +      * driver data through this dw_hdmi_plat_data.
> > +      */
> > +     void *priv_audio;
> > +
> >       /* Platform-specific audio enable/disable (optional) */
> >       void (*enable_audio)(struct dw_hdmi *hdmi, int channel,
> >                            int width, int rate, int non_pcm, int iec958);
> >
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>

Re: [PATCH v3 6/6] arm64: dts: imx8mp: Add hdmi parallel audio interface node

[Shengjiu Wang]

On Tue, Aug 5, 2025 at 3:10 PM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Am Montag, 4. August 2025, 12:47:22 CEST schrieb Shengjiu Wang:
> > The HDMI TX Parallel Audio Interface (HTX_PAI) is a bridge between the
> > Audio Subsystem to the HDMI TX Controller.
> >
> > Shrink register map size of hdmi_pvi to avoid overlapped hdmi_pai device.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  arch/arm64/boot/dts/freescale/imx8mp-evk.dts |  4 +++
> >  arch/arm64/boot/dts/freescale/imx8mp.dtsi    | 28 +++++++++++++++++++-
>
> Please separate commits for SoC and board files. Thanks

Ok.

Best regards
Shengjiu Wang
>
> Best regards,
> Alexander
>
> >  2 files changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> > index c0cc5611048e..cc9351a5bd65 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> > @@ -442,6 +442,10 @@ &flexcan2 {
> >       status = "disabled";/* can2 pin conflict with pdm */
> >  };
> >
> > +&hdmi_pai {
> > +     status = "okay";
> > +};
> > +
> >  &hdmi_pvi {
> >       status = "okay";
> >  };
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > index 841d155685ee..00d8474bd1b1 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > @@ -2066,7 +2066,7 @@ irqsteer_hdmi: interrupt-controller@32fc2000 {
> >
> >                       hdmi_pvi: display-bridge@32fc4000 {
> >                               compatible = "fsl,imx8mp-hdmi-pvi";
> > -                             reg = <0x32fc4000 0x1000>;
> > +                             reg = <0x32fc4000 0x800>;
> >                               interrupt-parent = <&irqsteer_hdmi>;
> >                               interrupts = <12>;
> >                               power-domains = <&hdmi_blk_ctrl IMX8MP_HDMIBLK_PD_PVI>;
> > @@ -2092,6 +2092,24 @@ pvi_to_hdmi_tx: endpoint {
> >                               };
> >                       };
> >
> > +                     hdmi_pai: audio-bridge@32fc4800 {
> > +                             compatible = "fsl,imx8mp-hdmi-pai";
> > +                             reg = <0x32fc4800 0x800>;
> > +                             interrupt-parent = <&irqsteer_hdmi>;
> > +                             interrupts = <14>;
> > +                             clocks = <&clk IMX8MP_CLK_HDMI_APB>;
> > +                             clock-names = "apb";
> > +                             power-domains = <&hdmi_blk_ctrl IMX8MP_HDMIBLK_PD_PAI>;
> > +                             status = "disabled";
> > +
> > +                             port {
> > +
> > +                                     pai_to_hdmi_tx: endpoint {
> > +                                             remote-endpoint = <&hdmi_tx_from_pai>;
> > +                                     };
> > +                             };
> > +                     };
> > +
> >                       lcdif3: display-controller@32fc6000 {
> >                               compatible = "fsl,imx8mp-lcdif";
> >                               reg = <0x32fc6000 0x1000>;
> > @@ -2143,6 +2161,14 @@ port@1 {
> >                                               reg = <1>;
> >                                               /* Point endpoint to the HDMI connector */
> >                                       };
> > +
> > +                                     port@2 {
> > +                                             reg = <2>;
> > +
> > +                                             hdmi_tx_from_pai: endpoint {
> > +                                                     remote-endpoint = <&pai_to_hdmi_tx>;
> > +                                             };
> > +                                     };
> >                               };
> >                       };
> >
> >
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Shengjiu Wang]

On Tue, Aug 5, 2025 at 4:55 PM Liu Ying <victor.liu@nxp.com> wrote:
>
> On 08/04/2025, Shengjiu Wang wrote:
> > The HDMI TX Parallel Audio Interface (HTX_PAI) is a digital module that
> > acts as the bridge between the Audio Subsystem to the HDMI TX Controller.
> > This IP block is found in the HDMI subsystem of the i.MX8MP SoC.
> >
> > Data received from the audio subsystem can have an arbitrary component
> > ordering. The HTX_PAI block has integrated muxing options to select which
> > sections of the 32-bit input data word will be mapped to each IEC60958
> > field. The HTX_PAI_FIELD_CTRL register contains mux selects to
> > individually select P,C,U,V,Data, and Preamble.
> >
> > Use component helper that imx8mp-hdmi-tx will be aggregate driver,
>
> s/that/so that/
>
> > imx8mp-hdmi-pai will be component driver, then imx8mp-hdmi-pai can use
> > bind() ops to get the plat_data from imx8mp-hdmi-tx device.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  drivers/gpu/drm/bridge/imx/Kconfig           |   8 +
> >  drivers/gpu/drm/bridge/imx/Makefile          |   1 +
> >  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c | 205 +++++++++++++++++++
> >  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c  |  55 +++++
> >  include/drm/bridge/dw_hdmi.h                 |   6 +
> >  5 files changed, 275 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> >
> > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig
> > index 9a480c6abb85..6c1a8bc5d4a0 100644
> > --- a/drivers/gpu/drm/bridge/imx/Kconfig
> > +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> > @@ -18,12 +18,20 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE
> >       depends on OF
> >       depends on COMMON_CLK
> >       select DRM_DW_HDMI
> > +     imply DRM_IMX8MP_HDMI_PAI
> >       imply DRM_IMX8MP_HDMI_PVI
> >       imply PHY_FSL_SAMSUNG_HDMI_PHY
> >       help
> >         Choose this to enable support for the internal HDMI encoder found
> >         on the i.MX8MP SoC.
> >
> > +config DRM_IMX8MP_HDMI_PAI
> > +     tristate "Freescale i.MX8MP HDMI PAI bridge support"
> > +     depends on OF
>
> select REGMAP
> select REGMAP_MMIO
>
> > +     help
> > +       Choose this to enable support for the internal HDMI TX Parallel
> > +       Audio Interface found on the Freescale i.MX8MP SoC.
> > +
> >  config DRM_IMX8MP_HDMI_PVI
> >       tristate "Freescale i.MX8MP HDMI PVI bridge support"
> >       depends on OF
> > diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile
> > index dd5d48584806..8d01fda25451 100644
> > --- a/drivers/gpu/drm/bridge/imx/Makefile
> > +++ b/drivers/gpu/drm/bridge/imx/Makefile
> > @@ -1,6 +1,7 @@
> >  obj-$(CONFIG_DRM_IMX_LDB_HELPER) += imx-ldb-helper.o
> >  obj-$(CONFIG_DRM_IMX_LEGACY_BRIDGE) += imx-legacy-bridge.o
> >  obj-$(CONFIG_DRM_IMX8MP_DW_HDMI_BRIDGE) += imx8mp-hdmi-tx.o
> > +obj-$(CONFIG_DRM_IMX8MP_HDMI_PAI) += imx8mp-hdmi-pai.o
> >  obj-$(CONFIG_DRM_IMX8MP_HDMI_PVI) += imx8mp-hdmi-pvi.o
> >  obj-$(CONFIG_DRM_IMX8QM_LDB) += imx8qm-ldb.o
> >  obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
> > diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> > new file mode 100644
> > index 000000000000..9002974073ca
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> > @@ -0,0 +1,205 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2025 NXP
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/component.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
>
> #include <linux/regmap.h>
>
> > +#include <drm/bridge/dw_hdmi.h>
> > +#include <sound/asoundef.h>
> > +
> > +#define HTX_PAI_CTRL                 0x00
> > +#define   ENABLE                     BIT(0)
> > +
> > +#define HTX_PAI_CTRL_EXT             0x04
> > +#define   WTMK_HIGH_MASK             GENMASK(31, 24)
> > +#define   WTMK_LOW_MASK                      GENMASK(23, 16)
> > +#define   NUM_CH_MASK                        GENMASK(10, 8)
>
> Add NUM_CH(n) and use it when programming HTX_PAI_CTRL_EXT.
> #define NUM_CH(n)                       FIELD_PREP(NUM_CH_MASK, (n - 1))
>
> > +#define   WTMK_HIGH(n)                       FIELD_PREP(WTMK_HIGH_MASK, (n))
> > +#define   WTMK_LOW(n)                        FIELD_PREP(WTMK_LOW_MASK, (n))
> > +
> > +#define HTX_PAI_FIELD_CTRL           0x08
> > +#define   B_FILT                     BIT(31)
> > +#define   PARITY_EN                  BIT(30)
> > +#define   END_SEL                    BIT(29)
>
> The above 3 bits are unused.  Drop.
>
> > +#define   PRE_SEL                    GENMASK(28, 24)
> > +#define   D_SEL                              GENMASK(23, 20)
> > +#define   V_SEL                              GENMASK(19, 15)
> > +#define   U_SEL                              GENMASK(14, 10)
> > +#define   C_SEL                              GENMASK(9, 5)
> > +#define   P_SEL                              GENMASK(4, 0)
> > +
> > +#define HTX_PAI_STAT                 0x0c
> > +#define HTX_PAI_IRQ_NOMASK           0x10
> > +#define HTX_PAI_IRQ_MASKED           0x14
> > +#define HTX_PAI_IRQ_MASK             0x18
>
> The above 4 registers are not pratically used.  Drop.
>
> > +
> > +struct imx8mp_hdmi_pai {
> > +     struct device   *dev;
> > +     struct regmap   *regmap;
> > +};
> > +
> > +static void imx8mp_hdmi_pai_enable(struct dw_hdmi *dw_hdmi, int channel,
> > +                                int width, int rate, int non_pcm,
> > +                                int iec958)
> > +{
> > +     const struct dw_hdmi_plat_data *pdata = dw_hdmi_to_plat_data(dw_hdmi);
> > +     struct imx8mp_hdmi_pai *hdmi_pai = (struct imx8mp_hdmi_pai *)pdata->priv_audio;
>
> I don't think you need to convert type explicitly.  Same for the other
> explicit conversions in this driver.
>
> > +     int val;
> > +
> > +     /* PAI set control extended */
> > +     val =  WTMK_HIGH(3) | WTMK_LOW(3);
> > +     val |= FIELD_PREP(NUM_CH_MASK, channel - 1);
> > +     regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL_EXT, val);
> > +
> > +     /* IEC60958 format */
> > +     if (iec958) {
> > +             val = FIELD_PREP_CONST(P_SEL,
> > +                                    __bf_shf(IEC958_SUBFRAME_PARITY));
> > +             val |= FIELD_PREP_CONST(C_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_CHANNEL_STATUS));
> > +             val |= FIELD_PREP_CONST(U_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_USER_DATA));
> > +             val |= FIELD_PREP_CONST(V_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_VALIDITY));
> > +             val |= FIELD_PREP_CONST(D_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_SAMPLE_24_MASK));
> > +             val |= FIELD_PREP_CONST(PRE_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_PREAMBLE_MASK));
> > +     } else {
> > +             /* PCM choose 24bit*/
> > +             val = FIELD_PREP(D_SEL, width - 24);
>
> Why 'width - 24'?  Can it be expressed by a helper or macro?
>
> > +     }
> > +
> > +     regmap_write(hdmi_pai->regmap, HTX_PAI_FIELD_CTRL, val);
> > +
> > +     /* PAI start running */
> > +     regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL, ENABLE);
> > +}
> > +
> > +static void imx8mp_hdmi_pai_disable(struct dw_hdmi *dw_hdmi)
> > +{
> > +     const struct dw_hdmi_plat_data *pdata = dw_hdmi_to_plat_data(dw_hdmi);
> > +     struct imx8mp_hdmi_pai *hdmi_pai = (struct imx8mp_hdmi_pai *)pdata->priv_audio;
> > +
> > +     /* Stop PAI */
> > +     regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL, 0);
> > +}
> > +
> > +static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
> > +{
> > +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> > +     struct imx8mp_hdmi_pai *hdmi_pai;
> > +
> > +     hdmi_pai = dev_get_drvdata(dev);
> > +
> > +     plat_data->enable_audio = imx8mp_hdmi_pai_enable;
> > +     plat_data->disable_audio = imx8mp_hdmi_pai_disable;
> > +     plat_data->priv_audio = hdmi_pai;
> > +
> > +     return 0;
> > +}
> > +
> > +static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
> > +{
> > +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> > +
> > +     plat_data->enable_audio = NULL;
> > +     plat_data->disable_audio = NULL;
> > +     plat_data->priv_audio = NULL;
>
> Do you really need to set these ptrs to NULL?

yes.  below code in dw-hdmi.c use the pdata->enable_audio as condition.

        if (pdata->enable_audio)
                pdata->enable_audio(hdmi,
                                    hdmi->channels,
                                    hdmi->sample_width,
                                    hdmi->sample_rate,
                                    hdmi->sample_non_pcm,
                                    hdmi->sample_iec958);


>
> > +}
> > +
> > +static const struct component_ops imx8mp_hdmi_pai_ops = {
> > +     .bind   = imx8mp_hdmi_pai_bind,
> > +     .unbind = imx8mp_hdmi_pai_unbind,
> > +};
> > +
> > +static bool imx8mp_hdmi_pai_volatile_reg(struct device *dev, unsigned int reg)
> > +{
> > +     switch (reg) {
> > +     case HTX_PAI_IRQ_NOMASK:
> > +     case HTX_PAI_IRQ_MASKED:
> > +             return true;
> > +     default:
> > +             return false;
> > +     }
> > +}
> > +
> > +static bool imx8mp_hdmi_pai_writeable_reg(struct device *dev, unsigned int reg)
> > +{
> > +     switch (reg) {
> > +     case HTX_PAI_CTRL:
> > +     case HTX_PAI_CTRL_EXT:
> > +     case HTX_PAI_FIELD_CTRL:
> > +     case HTX_PAI_IRQ_NOMASK:
> > +     case HTX_PAI_IRQ_MASKED:
> > +     case HTX_PAI_IRQ_MASK:
> > +             return true;
> > +     default:
> > +             return false;
> > +     }
> > +}
> > +
> > +static const struct regmap_config imx8mp_hdmi_pai_regmap_config = {
> > +     .reg_bits = 32,
> > +     .reg_stride = 4,
> > +     .val_bits = 32,
> > +     .max_register = HTX_PAI_IRQ_MASK,
> > +     .volatile_reg = imx8mp_hdmi_pai_volatile_reg,
> > +     .writeable_reg = imx8mp_hdmi_pai_writeable_reg,
> > +};
> > +
> > +static int imx8mp_hdmi_pai_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct imx8mp_hdmi_pai *hdmi_pai;
> > +     struct resource *res;
> > +     void __iomem *base;
> > +
> > +     hdmi_pai = devm_kzalloc(dev, sizeof(*hdmi_pai), GFP_KERNEL);
> > +     if (!hdmi_pai)
> > +             return -ENOMEM;
> > +
> > +     base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     hdmi_pai->dev = dev;
> > +
> > +     hdmi_pai->regmap = devm_regmap_init_mmio(dev, base, &imx8mp_hdmi_pai_regmap_config);
>
> Now that DT binding says there is an APB clock, use devm_regmap_init_mmio_clk()
> to ensure registers can be accessed with clock enabled even via debugfs.
>
> > +     if (IS_ERR(hdmi_pai->regmap)) {
> > +             dev_err(dev, "regmap init failed\n");
> > +             return PTR_ERR(hdmi_pai->regmap);
> > +     }
> > +
> > +     dev_set_drvdata(dev, hdmi_pai);
> > +
> > +     return component_add(dev, &imx8mp_hdmi_pai_ops);
>
> Imagine that users could enable this driver without enabling imx8mp-hdmi-tx
> driver, you may add the component in this probe() callback only and move all
> the other stuff to bind() callback to avoid unnecessary things being done here.

component helper functions don't have such dependency that the aggregate
driver or component driver must be probed or not.  if imx8mp-hdmi-tx is not
enabled, there is no problem, just the bind() callback is not called.

>
> > +}
> > +
> > +static void imx8mp_hdmi_pai_remove(struct platform_device *pdev)
> > +{
> > +     component_del(&pdev->dev, &imx8mp_hdmi_pai_ops);
> > +}
> > +
> > +static const struct of_device_id imx8mp_hdmi_pai_of_table[] = {
> > +     { .compatible = "fsl,imx8mp-hdmi-pai" },
> > +     { /* Sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx8mp_hdmi_pai_of_table);
> > +
> > +static struct platform_driver imx8mp_hdmi_pai_platform_driver = {
> > +     .probe          = imx8mp_hdmi_pai_probe,
> > +     .remove         = imx8mp_hdmi_pai_remove,
> > +     .driver         = {
> > +             .name   = "imx8mp-hdmi-pai",
> > +             .of_match_table = imx8mp_hdmi_pai_of_table,
> > +     },
> > +};
> > +module_platform_driver(imx8mp_hdmi_pai_platform_driver);
> > +
> > +MODULE_DESCRIPTION("i.MX8MP HDMI PAI driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> > index 1e7a789ec289..ee08084d2394 100644
> > --- a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> > +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> > @@ -5,11 +5,13 @@
> >   */
> >
> >  #include <linux/clk.h>
> > +#include <linux/component.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> >  #include <drm/bridge/dw_hdmi.h>
> >  #include <drm/drm_modes.h>
> > +#include <drm/drm_of.h>
> >
> >  struct imx8mp_hdmi {
> >       struct dw_hdmi_plat_data plat_data;
> > @@ -79,11 +81,46 @@ static const struct dw_hdmi_phy_ops imx8mp_hdmi_phy_ops = {
> >       .update_hpd     = dw_hdmi_phy_update_hpd,
> >  };
> >
> > +static int imx8mp_dw_hdmi_bind(struct device *dev)
> > +{
> > +     struct dw_hdmi_plat_data *plat_data;
> > +     struct imx8mp_hdmi *hdmi;
> > +     int ret;
> > +
> > +     hdmi = dev_get_drvdata(dev);
> > +     plat_data = &hdmi->plat_data;
> > +
> > +     ret = component_bind_all(dev, plat_data);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "component_bind_all failed!\n");
>
> As component_bind_all() would bind imx8mp-hdmi-pai and hence set
> {enable,disable}_audio callbacks, you need to call dw_hdmi_probe() after
> component_bind_all() instead of too early in probe() callback.

There is no such dependency.
Maybe you mixed the hdmi->enable_audio() with pdata->enable_audio().

>
> > +
> > +     return 0;
> > +}
> > +
> > +static void imx8mp_dw_hdmi_unbind(struct device *dev)
> > +{
> > +     struct dw_hdmi_plat_data *plat_data;
> > +     struct imx8mp_hdmi *hdmi;
> > +
> > +     hdmi = dev_get_drvdata(dev);
> > +     plat_data = &hdmi->plat_data;
> > +
> > +     component_unbind_all(dev, plat_data);
> > +}
> > +
> > +static const struct component_master_ops imx8mp_dw_hdmi_ops = {
> > +     .bind   = imx8mp_dw_hdmi_bind,
> > +     .unbind = imx8mp_dw_hdmi_unbind,
> > +};
> > +
> >  static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
> >  {
> >       struct device *dev = &pdev->dev;
> >       struct dw_hdmi_plat_data *plat_data;
> > +     struct component_match *match;
>
> As Alexander pointed out, this needs to be set to NULL.
> See how other drivers which are component masters do.
>
> > +     struct device_node *remote;
> >       struct imx8mp_hdmi *hdmi;
> > +     int ret;
> >
> >       hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
> >       if (!hdmi)
> > @@ -108,6 +145,22 @@ static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
> >
> >       platform_set_drvdata(pdev, hdmi);
> >
> > +     /* port@2 is for hdmi_pai device */
> > +     remote = of_graph_get_remote_node(pdev->dev.of_node, 2, 0);
> > +     if (remote && of_device_is_available(remote)) {
>
> Doesn't of_graph_get_remote_node() ensure that remote is avaiable?

No.  'remote' is the node,  not the 'device'.

>
> > +             drm_of_component_match_add(dev, &match, component_compare_of, remote);
> > +
> > +             of_node_put(remote);
> > +
> > +             ret = component_master_add_with_match(dev, &imx8mp_dw_hdmi_ops, match);
> > +             if (ret)
> > +                     dev_warn(dev, "Unable to register aggregate driver\n");
> > +             /*
> > +              * This audio function is optional for avoid blocking display.
> > +              * So just print warning message and no error is returned.
>
> No, since PAI node is available here, it has to be bound.  Yet you still need
> to properly handle the case where PAI node is inavailable.

This is for aggregate driver registration,  not for bind()

The bind() is called after both drivers have been registered.  again there is no
dependency for both aggregate driver and component driver should be
registered or probed.

best regards
shengjiu wang

>
> > +              */
> > +     }
> > +
> >       return 0;
> >  }
> >
> > @@ -115,6 +168,8 @@ static void imx8mp_dw_hdmi_remove(struct platform_device *pdev)
> >  {
> >       struct imx8mp_hdmi *hdmi = platform_get_drvdata(pdev);
> >
> > +     component_master_del(&pdev->dev, &imx8mp_dw_hdmi_ops);
> > +
> >       dw_hdmi_remove(hdmi->dw_hdmi);
> >  }
> >
> > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> > index 095cdd9b7424..336f062e1f9d 100644
> > --- a/include/drm/bridge/dw_hdmi.h
> > +++ b/include/drm/bridge/dw_hdmi.h
> > @@ -143,6 +143,12 @@ struct dw_hdmi_plat_data {
> >                                          const struct drm_display_info *info,
> >                                          const struct drm_display_mode *mode);
> >
> > +     /*
> > +      * priv_audio is specially used for additional audio device to get
> > +      * driver data through this dw_hdmi_plat_data.
> > +      */
> > +     void *priv_audio;
> > +
> >       /* Platform-specific audio enable/disable (optional) */
> >       void (*enable_audio)(struct dw_hdmi *hdmi, int channel,
> >                            int width, int rate, int non_pcm, int iec958);
>
>
> --
> Regards,
> Liu Ying

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Shengjiu Wang]

On Tue, Aug 5, 2025 at 4:55 PM Liu Ying <victor.liu@nxp.com> wrote:
>
> On 08/04/2025, Shengjiu Wang wrote:
> > The HDMI TX Parallel Audio Interface (HTX_PAI) is a digital module that
> > acts as the bridge between the Audio Subsystem to the HDMI TX Controller.
> > This IP block is found in the HDMI subsystem of the i.MX8MP SoC.
> >
> > Data received from the audio subsystem can have an arbitrary component
> > ordering. The HTX_PAI block has integrated muxing options to select which
> > sections of the 32-bit input data word will be mapped to each IEC60958
> > field. The HTX_PAI_FIELD_CTRL register contains mux selects to
> > individually select P,C,U,V,Data, and Preamble.
> >
> > Use component helper that imx8mp-hdmi-tx will be aggregate driver,
>
> s/that/so that/
>
> > imx8mp-hdmi-pai will be component driver, then imx8mp-hdmi-pai can use
> > bind() ops to get the plat_data from imx8mp-hdmi-tx device.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  drivers/gpu/drm/bridge/imx/Kconfig           |   8 +
> >  drivers/gpu/drm/bridge/imx/Makefile          |   1 +
> >  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c | 205 +++++++++++++++++++
> >  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c  |  55 +++++
> >  include/drm/bridge/dw_hdmi.h                 |   6 +
> >  5 files changed, 275 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> >
> > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig
> > index 9a480c6abb85..6c1a8bc5d4a0 100644
> > --- a/drivers/gpu/drm/bridge/imx/Kconfig
> > +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> > @@ -18,12 +18,20 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE
> >       depends on OF
> >       depends on COMMON_CLK
> >       select DRM_DW_HDMI
> > +     imply DRM_IMX8MP_HDMI_PAI
> >       imply DRM_IMX8MP_HDMI_PVI
> >       imply PHY_FSL_SAMSUNG_HDMI_PHY
> >       help
> >         Choose this to enable support for the internal HDMI encoder found
> >         on the i.MX8MP SoC.
> >
> > +config DRM_IMX8MP_HDMI_PAI
> > +     tristate "Freescale i.MX8MP HDMI PAI bridge support"
> > +     depends on OF
>
> select REGMAP
> select REGMAP_MMIO


will add them.

>
> > +     help
> > +       Choose this to enable support for the internal HDMI TX Parallel
> > +       Audio Interface found on the Freescale i.MX8MP SoC.
> > +
> >  config DRM_IMX8MP_HDMI_PVI
> >       tristate "Freescale i.MX8MP HDMI PVI bridge support"
> >       depends on OF
> > diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile
> > index dd5d48584806..8d01fda25451 100644
> > --- a/drivers/gpu/drm/bridge/imx/Makefile
> > +++ b/drivers/gpu/drm/bridge/imx/Makefile
> > @@ -1,6 +1,7 @@
> >  obj-$(CONFIG_DRM_IMX_LDB_HELPER) += imx-ldb-helper.o
> >  obj-$(CONFIG_DRM_IMX_LEGACY_BRIDGE) += imx-legacy-bridge.o
> >  obj-$(CONFIG_DRM_IMX8MP_DW_HDMI_BRIDGE) += imx8mp-hdmi-tx.o
> > +obj-$(CONFIG_DRM_IMX8MP_HDMI_PAI) += imx8mp-hdmi-pai.o
> >  obj-$(CONFIG_DRM_IMX8MP_HDMI_PVI) += imx8mp-hdmi-pvi.o
> >  obj-$(CONFIG_DRM_IMX8QM_LDB) += imx8qm-ldb.o
> >  obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
> > diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> > new file mode 100644
> > index 000000000000..9002974073ca
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> > @@ -0,0 +1,205 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2025 NXP
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/component.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
>
> #include <linux/regmap.h>

ok,
>
> > +#include <drm/bridge/dw_hdmi.h>
> > +#include <sound/asoundef.h>
> > +
> > +#define HTX_PAI_CTRL                 0x00
> > +#define   ENABLE                     BIT(0)
> > +
> > +#define HTX_PAI_CTRL_EXT             0x04
> > +#define   WTMK_HIGH_MASK             GENMASK(31, 24)
> > +#define   WTMK_LOW_MASK                      GENMASK(23, 16)
> > +#define   NUM_CH_MASK                        GENMASK(10, 8)
>
> Add NUM_CH(n) and use it when programming HTX_PAI_CTRL_EXT.
> #define NUM_CH(n)                       FIELD_PREP(NUM_CH_MASK, (n - 1))

Ok, will add it.

>
> > +#define   WTMK_HIGH(n)                       FIELD_PREP(WTMK_HIGH_MASK, (n))
> > +#define   WTMK_LOW(n)                        FIELD_PREP(WTMK_LOW_MASK, (n))
> > +
> > +#define HTX_PAI_FIELD_CTRL           0x08
> > +#define   B_FILT                     BIT(31)
> > +#define   PARITY_EN                  BIT(30)
> > +#define   END_SEL                    BIT(29)
>
> The above 3 bits are unused.  Drop.

Ok.

>
> > +#define   PRE_SEL                    GENMASK(28, 24)
> > +#define   D_SEL                              GENMASK(23, 20)
> > +#define   V_SEL                              GENMASK(19, 15)
> > +#define   U_SEL                              GENMASK(14, 10)
> > +#define   C_SEL                              GENMASK(9, 5)
> > +#define   P_SEL                              GENMASK(4, 0)
> > +
> > +#define HTX_PAI_STAT                 0x0c
> > +#define HTX_PAI_IRQ_NOMASK           0x10
> > +#define HTX_PAI_IRQ_MASKED           0x14
> > +#define HTX_PAI_IRQ_MASK             0x18
>
> The above 4 registers are not pratically used.  Drop.

They are used by regmap to make a full definition.

>
> > +
> > +struct imx8mp_hdmi_pai {
> > +     struct device   *dev;
> > +     struct regmap   *regmap;
> > +};
> > +
> > +static void imx8mp_hdmi_pai_enable(struct dw_hdmi *dw_hdmi, int channel,
> > +                                int width, int rate, int non_pcm,
> > +                                int iec958)
> > +{
> > +     const struct dw_hdmi_plat_data *pdata = dw_hdmi_to_plat_data(dw_hdmi);
> > +     struct imx8mp_hdmi_pai *hdmi_pai = (struct imx8mp_hdmi_pai *)pdata->priv_audio;
>
> I don't think you need to convert type explicitly.  Same for the other
> explicit conversions in this driver.

ok, can be removed.
>
> > +     int val;
> > +
> > +     /* PAI set control extended */
> > +     val =  WTMK_HIGH(3) | WTMK_LOW(3);
> > +     val |= FIELD_PREP(NUM_CH_MASK, channel - 1);
> > +     regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL_EXT, val);
> > +
> > +     /* IEC60958 format */
> > +     if (iec958) {
> > +             val = FIELD_PREP_CONST(P_SEL,
> > +                                    __bf_shf(IEC958_SUBFRAME_PARITY));
> > +             val |= FIELD_PREP_CONST(C_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_CHANNEL_STATUS));
> > +             val |= FIELD_PREP_CONST(U_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_USER_DATA));
> > +             val |= FIELD_PREP_CONST(V_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_VALIDITY));
> > +             val |= FIELD_PREP_CONST(D_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_SAMPLE_24_MASK));
> > +             val |= FIELD_PREP_CONST(PRE_SEL,
> > +                                     __bf_shf(IEC958_SUBFRAME_PREAMBLE_MASK));
> > +     } else {
> > +             /* PCM choose 24bit*/
> > +             val = FIELD_PREP(D_SEL, width - 24);
>
> Why 'width - 24'?  Can it be expressed by a helper or macro?

                /*
                 * The allowed width are 24bit and 32bit, as they are
supported by
                 * aud2htx module.
                 * for 24bit, D_SEL = 0, select all the bits.
                 * for 32bit, D_SEL = 8, select the MSB.
                 */
will add such comments.

best regards
Shengjiu wang
>
> > +     }
> > +
> > +     regmap_write(hdmi_pai->regmap, HTX_PAI_FIELD_CTRL, val);
> > +
> > +     /* PAI start running */
> > +     regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL, ENABLE);
> > +}
> > +
> > +static void imx8mp_hdmi_pai_disable(struct dw_hdmi *dw_hdmi)
> > +{
> > +     const struct dw_hdmi_plat_data *pdata = dw_hdmi_to_plat_data(dw_hdmi);
> > +     struct imx8mp_hdmi_pai *hdmi_pai = (struct imx8mp_hdmi_pai *)pdata->priv_audio;
> > +
> > +     /* Stop PAI */
> > +     regmap_write(hdmi_pai->regmap, HTX_PAI_CTRL, 0);
> > +}
> > +
> > +static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
> > +{
> > +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> > +     struct imx8mp_hdmi_pai *hdmi_pai;
> > +
> > +     hdmi_pai = dev_get_drvdata(dev);
> > +
> > +     plat_data->enable_audio = imx8mp_hdmi_pai_enable;
> > +     plat_data->disable_audio = imx8mp_hdmi_pai_disable;
> > +     plat_data->priv_audio = hdmi_pai;
> > +
> > +     return 0;
> > +}
> > +
> > +static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
> > +{
> > +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> > +
> > +     plat_data->enable_audio = NULL;
> > +     plat_data->disable_audio = NULL;
> > +     plat_data->priv_audio = NULL;
>
> Do you really need to set these ptrs to NULL?
>
> > +}
> > +
> > +static const struct component_ops imx8mp_hdmi_pai_ops = {
> > +     .bind   = imx8mp_hdmi_pai_bind,
> > +     .unbind = imx8mp_hdmi_pai_unbind,
> > +};
> > +
> > +static bool imx8mp_hdmi_pai_volatile_reg(struct device *dev, unsigned int reg)
> > +{
> > +     switch (reg) {
> > +     case HTX_PAI_IRQ_NOMASK:
> > +     case HTX_PAI_IRQ_MASKED:
> > +             return true;
> > +     default:
> > +             return false;
> > +     }
> > +}
> > +
> > +static bool imx8mp_hdmi_pai_writeable_reg(struct device *dev, unsigned int reg)
> > +{
> > +     switch (reg) {
> > +     case HTX_PAI_CTRL:
> > +     case HTX_PAI_CTRL_EXT:
> > +     case HTX_PAI_FIELD_CTRL:
> > +     case HTX_PAI_IRQ_NOMASK:
> > +     case HTX_PAI_IRQ_MASKED:
> > +     case HTX_PAI_IRQ_MASK:
> > +             return true;
> > +     default:
> > +             return false;
> > +     }
> > +}
> > +
> > +static const struct regmap_config imx8mp_hdmi_pai_regmap_config = {
> > +     .reg_bits = 32,
> > +     .reg_stride = 4,
> > +     .val_bits = 32,
> > +     .max_register = HTX_PAI_IRQ_MASK,
> > +     .volatile_reg = imx8mp_hdmi_pai_volatile_reg,
> > +     .writeable_reg = imx8mp_hdmi_pai_writeable_reg,
> > +};
> > +
> > +static int imx8mp_hdmi_pai_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct imx8mp_hdmi_pai *hdmi_pai;
> > +     struct resource *res;
> > +     void __iomem *base;
> > +
> > +     hdmi_pai = devm_kzalloc(dev, sizeof(*hdmi_pai), GFP_KERNEL);
> > +     if (!hdmi_pai)
> > +             return -ENOMEM;
> > +
> > +     base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     hdmi_pai->dev = dev;
> > +
> > +     hdmi_pai->regmap = devm_regmap_init_mmio(dev, base, &imx8mp_hdmi_pai_regmap_config);
>
> Now that DT binding says there is an APB clock, use devm_regmap_init_mmio_clk()
> to ensure registers can be accessed with clock enabled even via debugfs.
>
> > +     if (IS_ERR(hdmi_pai->regmap)) {
> > +             dev_err(dev, "regmap init failed\n");
> > +             return PTR_ERR(hdmi_pai->regmap);
> > +     }
> > +
> > +     dev_set_drvdata(dev, hdmi_pai);
> > +
> > +     return component_add(dev, &imx8mp_hdmi_pai_ops);
>
> Imagine that users could enable this driver without enabling imx8mp-hdmi-tx
> driver, you may add the component in this probe() callback only and move all
> the other stuff to bind() callback to avoid unnecessary things being done here.
>
> > +}
> > +
> > +static void imx8mp_hdmi_pai_remove(struct platform_device *pdev)
> > +{
> > +     component_del(&pdev->dev, &imx8mp_hdmi_pai_ops);
> > +}
> > +
> > +static const struct of_device_id imx8mp_hdmi_pai_of_table[] = {
> > +     { .compatible = "fsl,imx8mp-hdmi-pai" },
> > +     { /* Sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx8mp_hdmi_pai_of_table);
> > +
> > +static struct platform_driver imx8mp_hdmi_pai_platform_driver = {
> > +     .probe          = imx8mp_hdmi_pai_probe,
> > +     .remove         = imx8mp_hdmi_pai_remove,
> > +     .driver         = {
> > +             .name   = "imx8mp-hdmi-pai",
> > +             .of_match_table = imx8mp_hdmi_pai_of_table,
> > +     },
> > +};
> > +module_platform_driver(imx8mp_hdmi_pai_platform_driver);
> > +
> > +MODULE_DESCRIPTION("i.MX8MP HDMI PAI driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> > index 1e7a789ec289..ee08084d2394 100644
> > --- a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> > +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> > @@ -5,11 +5,13 @@
> >   */
> >
> >  #include <linux/clk.h>
> > +#include <linux/component.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> >  #include <drm/bridge/dw_hdmi.h>
> >  #include <drm/drm_modes.h>
> > +#include <drm/drm_of.h>
> >
> >  struct imx8mp_hdmi {
> >       struct dw_hdmi_plat_data plat_data;
> > @@ -79,11 +81,46 @@ static const struct dw_hdmi_phy_ops imx8mp_hdmi_phy_ops = {
> >       .update_hpd     = dw_hdmi_phy_update_hpd,
> >  };
> >
> > +static int imx8mp_dw_hdmi_bind(struct device *dev)
> > +{
> > +     struct dw_hdmi_plat_data *plat_data;
> > +     struct imx8mp_hdmi *hdmi;
> > +     int ret;
> > +
> > +     hdmi = dev_get_drvdata(dev);
> > +     plat_data = &hdmi->plat_data;
> > +
> > +     ret = component_bind_all(dev, plat_data);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "component_bind_all failed!\n");
>
> As component_bind_all() would bind imx8mp-hdmi-pai and hence set
> {enable,disable}_audio callbacks, you need to call dw_hdmi_probe() after
> component_bind_all() instead of too early in probe() callback.
>
> > +
> > +     return 0;
> > +}
> > +
> > +static void imx8mp_dw_hdmi_unbind(struct device *dev)
> > +{
> > +     struct dw_hdmi_plat_data *plat_data;
> > +     struct imx8mp_hdmi *hdmi;
> > +
> > +     hdmi = dev_get_drvdata(dev);
> > +     plat_data = &hdmi->plat_data;
> > +
> > +     component_unbind_all(dev, plat_data);
> > +}
> > +
> > +static const struct component_master_ops imx8mp_dw_hdmi_ops = {
> > +     .bind   = imx8mp_dw_hdmi_bind,
> > +     .unbind = imx8mp_dw_hdmi_unbind,
> > +};
> > +
> >  static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
> >  {
> >       struct device *dev = &pdev->dev;
> >       struct dw_hdmi_plat_data *plat_data;
> > +     struct component_match *match;
>
> As Alexander pointed out, this needs to be set to NULL.
> See how other drivers which are component masters do.
>
> > +     struct device_node *remote;
> >       struct imx8mp_hdmi *hdmi;
> > +     int ret;
> >
> >       hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
> >       if (!hdmi)
> > @@ -108,6 +145,22 @@ static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
> >
> >       platform_set_drvdata(pdev, hdmi);
> >
> > +     /* port@2 is for hdmi_pai device */
> > +     remote = of_graph_get_remote_node(pdev->dev.of_node, 2, 0);
> > +     if (remote && of_device_is_available(remote)) {
>
> Doesn't of_graph_get_remote_node() ensure that remote is avaiable?
>
> > +             drm_of_component_match_add(dev, &match, component_compare_of, remote);
> > +
> > +             of_node_put(remote);
> > +
> > +             ret = component_master_add_with_match(dev, &imx8mp_dw_hdmi_ops, match);
> > +             if (ret)
> > +                     dev_warn(dev, "Unable to register aggregate driver\n");
> > +             /*
> > +              * This audio function is optional for avoid blocking display.
> > +              * So just print warning message and no error is returned.
>
> No, since PAI node is available here, it has to be bound.  Yet you still need
> to properly handle the case where PAI node is inavailable.
>
> > +              */
> > +     }
> > +
> >       return 0;
> >  }
> >
> > @@ -115,6 +168,8 @@ static void imx8mp_dw_hdmi_remove(struct platform_device *pdev)
> >  {
> >       struct imx8mp_hdmi *hdmi = platform_get_drvdata(pdev);
> >
> > +     component_master_del(&pdev->dev, &imx8mp_dw_hdmi_ops);
> > +
> >       dw_hdmi_remove(hdmi->dw_hdmi);
> >  }
> >
> > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> > index 095cdd9b7424..336f062e1f9d 100644
> > --- a/include/drm/bridge/dw_hdmi.h
> > +++ b/include/drm/bridge/dw_hdmi.h
> > @@ -143,6 +143,12 @@ struct dw_hdmi_plat_data {
> >                                          const struct drm_display_info *info,
> >                                          const struct drm_display_mode *mode);
> >
> > +     /*
> > +      * priv_audio is specially used for additional audio device to get
> > +      * driver data through this dw_hdmi_plat_data.
> > +      */
> > +     void *priv_audio;
> > +
> >       /* Platform-specific audio enable/disable (optional) */
> >       void (*enable_audio)(struct dw_hdmi *hdmi, int channel,
> >                            int width, int rate, int non_pcm, int iec958);
>
>
> --
> Regards,
> Liu Ying

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Liu Ying]

On 08/06/2025, Shengjiu Wang wrote:
> On Tue, Aug 5, 2025 at 4:55 PM Liu Ying <victor.liu@nxp.com> wrote:
>>
>> On 08/04/2025, Shengjiu Wang wrote:

[...]

>>> +static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
>>> +{
>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
>>> +     struct imx8mp_hdmi_pai *hdmi_pai;
>>> +
>>> +     hdmi_pai = dev_get_drvdata(dev);
>>> +
>>> +     plat_data->enable_audio = imx8mp_hdmi_pai_enable;
>>> +     plat_data->disable_audio = imx8mp_hdmi_pai_disable;
>>> +     plat_data->priv_audio = hdmi_pai;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
>>> +{
>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
>>> +
>>> +     plat_data->enable_audio = NULL;
>>> +     plat_data->disable_audio = NULL;
>>> +     plat_data->priv_audio = NULL;
>>
>> Do you really need to set these ptrs to NULL?
> 
> yes.  below code in dw-hdmi.c use the pdata->enable_audio as condition.

Note that this is all about tearing down components.
If this is done properly as the below snippet of pseudo-code, then
hdmi->{enable,disable}_audio() and pdata->{enable,disable}_audio() won't be
called after audio device is removed by dw_hdmi_remove().  So, it's unnecessary
to set these pointers to NULL here.

imx8mp_dw_hdmi_unbind()
{
   dw_hdmi_remove(); // platform_device_unregister(hdmi->audio);
   component_unbind_all(); //imx8mp_hdmi_pai_unbind()
}

BTW, I suggest the below snippet[1] to bind components.

imx8mp_dw_hdmi_bind()
{
   component_bind_all(); // imx8mp_hdmi_pai_bind()
                         //   set pdata->{enable,disable}_audio
   dw_hdmi_probe(); // hdmi->audio = platform_device_register_full(&pdevinfo);
}

> 
>         if (pdata->enable_audio)
>                 pdata->enable_audio(hdmi,
>                                     hdmi->channels,
>                                     hdmi->sample_width,
>                                     hdmi->sample_rate,
>                                     hdmi->sample_non_pcm,
>                                     hdmi->sample_iec958);
> 
> 
>>

[...]

>>> +     return component_add(dev, &imx8mp_hdmi_pai_ops);
>>
>> Imagine that users could enable this driver without enabling imx8mp-hdmi-tx
>> driver, you may add the component in this probe() callback only and move all
>> the other stuff to bind() callback to avoid unnecessary things being done here.
> 
> component helper functions don't have such dependency that the aggregate
> driver or component driver must be probed or not.  if imx8mp-hdmi-tx is not
> enabled, there is no problem, just the bind() callback is not called.

I meant I'd write imx8mp_hdmi_pai_probe() as below snippet and do all the
other stuff in imx8mp_hdmi_pai_bind().  This ensures minimum things are done
in imx8mp_hdmi_pai_probe() if imx8mp-hdmi-tx doesn't probe.

static int imx8mp_hdmi_pai_probe(struct platform_device *pdev)
{
        return component_add(&pdev->dev, &imx8mp_hdmi_pai_ops);
}

> 
>>
>>> +}
>>> +
>>> +static void imx8mp_hdmi_pai_remove(struct platform_device *pdev)
>>> +{
>>> +     component_del(&pdev->dev, &imx8mp_hdmi_pai_ops);
>>> +}
>>> +
>>> +static const struct of_device_id imx8mp_hdmi_pai_of_table[] = {
>>> +     { .compatible = "fsl,imx8mp-hdmi-pai" },
>>> +     { /* Sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, imx8mp_hdmi_pai_of_table);
>>> +
>>> +static struct platform_driver imx8mp_hdmi_pai_platform_driver = {
>>> +     .probe          = imx8mp_hdmi_pai_probe,
>>> +     .remove         = imx8mp_hdmi_pai_remove,
>>> +     .driver         = {
>>> +             .name   = "imx8mp-hdmi-pai",
>>> +             .of_match_table = imx8mp_hdmi_pai_of_table,
>>> +     },
>>> +};
>>> +module_platform_driver(imx8mp_hdmi_pai_platform_driver);
>>> +
>>> +MODULE_DESCRIPTION("i.MX8MP HDMI PAI driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
>>> index 1e7a789ec289..ee08084d2394 100644
>>> --- a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
>>> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
>>> @@ -5,11 +5,13 @@
>>>   */
>>>
>>>  #include <linux/clk.h>
>>> +#include <linux/component.h>
>>>  #include <linux/mod_devicetable.h>
>>>  #include <linux/module.h>
>>>  #include <linux/platform_device.h>
>>>  #include <drm/bridge/dw_hdmi.h>
>>>  #include <drm/drm_modes.h>
>>> +#include <drm/drm_of.h>
>>>
>>>  struct imx8mp_hdmi {
>>>       struct dw_hdmi_plat_data plat_data;
>>> @@ -79,11 +81,46 @@ static const struct dw_hdmi_phy_ops imx8mp_hdmi_phy_ops = {
>>>       .update_hpd     = dw_hdmi_phy_update_hpd,
>>>  };
>>>
>>> +static int imx8mp_dw_hdmi_bind(struct device *dev)
>>> +{
>>> +     struct dw_hdmi_plat_data *plat_data;
>>> +     struct imx8mp_hdmi *hdmi;
>>> +     int ret;
>>> +
>>> +     hdmi = dev_get_drvdata(dev);
>>> +     plat_data = &hdmi->plat_data;
>>> +
>>> +     ret = component_bind_all(dev, plat_data);
>>> +     if (ret)
>>> +             return dev_err_probe(dev, ret, "component_bind_all failed!\n");
>>
>> As component_bind_all() would bind imx8mp-hdmi-pai and hence set
>> {enable,disable}_audio callbacks, you need to call dw_hdmi_probe() after
>> component_bind_all() instead of too early in probe() callback.
> 
> There is no such dependency.
> Maybe you mixed the hdmi->enable_audio() with pdata->enable_audio().

As the above snippet[1] shows, once dw_hdmi_probe() registers audio device,
the audio device could be functional soon after audio driver probes, hence
hdmi->enable_audio() would be called and hence pdata->enable_audio() would
be called. So, you need to set pdata->enable_audio() before dw_hdmi_probe()
is called, otherwise pdata->enable_audio could be NULL when is called by
audio driver.

[...]

>>> +     remote = of_graph_get_remote_node(pdev->dev.of_node, 2, 0);
>>> +     if (remote && of_device_is_available(remote)) {
>>
>> Doesn't of_graph_get_remote_node() ensure that remote is avaiable?
> 
> No.  'remote' is the node,  not the 'device'.

See of_device_is_available() is called by of_graph_get_remote_node():

struct device_node *of_graph_get_remote_node(const struct device_node *node,     
                                             u32 port, u32 endpoint)             
{                                                                                
        struct device_node *endpoint_node, *remote;                              
                                                                                 
        endpoint_node = of_graph_get_endpoint_by_regs(node, port, endpoint);     
        if (!endpoint_node) {                                                    
                pr_debug("no valid endpoint (%d, %d) for node %pOF\n",           
                         port, endpoint, node);                                  
                return NULL;                                                     
        }                                                                        
                                                                                 
        remote = of_graph_get_remote_port_parent(endpoint_node);                 
        of_node_put(endpoint_node);                                              
        if (!remote) {                                                           
                pr_debug("no valid remote node\n");                              
                return NULL;                                                     
        }                                                                        
                                                                                 
        if (!of_device_is_available(remote)) {                                   
             ^~~~~~~~~~~~~~~~~~~~~~
                pr_debug("not available for remote node\n");                     
                of_node_put(remote);                                             
                return NULL;                                                     
        }                                                                        
                                                                                 
        return remote;                                                           
}                                                                                
EXPORT_SYMBOL(of_graph_get_remote_node);      

> 
>>
>>> +             drm_of_component_match_add(dev, &match, component_compare_of, remote);
>>> +
>>> +             of_node_put(remote);
>>> +
>>> +             ret = component_master_add_with_match(dev, &imx8mp_dw_hdmi_ops, match);
>>> +             if (ret)
>>> +                     dev_warn(dev, "Unable to register aggregate driver\n");
>>> +             /*
>>> +              * This audio function is optional for avoid blocking display.
>>> +              * So just print warning message and no error is returned.
>>
>> No, since PAI node is available here, it has to be bound.  Yet you still need
>> to properly handle the case where PAI node is inavailable.
> 
> This is for aggregate driver registration,  not for bind()
> 
> The bind() is called after both drivers have been registered.  again there is no
> dependency for both aggregate driver and component driver should be
> registered or probed.

Sorry for not being clear about my previous wording.  I meant since PAI node is
available here, component_master_add_with_match() must be called to register
the master and if it fails to register it, imx8mp_dw_hdmi_probe() should return
proper error code, not 0.

-- 
Regards,
Liu Ying

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Liu Ying]

On 08/06/2025, Shengjiu Wang wrote:
> On Tue, Aug 5, 2025 at 4:55 PM Liu Ying <victor.liu@nxp.com> wrote:
>>
>> On 08/04/2025, Shengjiu Wang wrote:

[...]

>>> +#define HTX_PAI_STAT                 0x0c
>>> +#define HTX_PAI_IRQ_NOMASK           0x10
>>> +#define HTX_PAI_IRQ_MASKED           0x14
>>> +#define HTX_PAI_IRQ_MASK             0x18
>>
>> The above 4 registers are not pratically used.  Drop.
> 
> They are used by regmap to make a full definition.

Yes, but they are not pratically used for interrupt support.
Once you add interrupt support in this driver, you may add these registers.

[...]

>>> +             /* PCM choose 24bit*/
>>> +             val = FIELD_PREP(D_SEL, width - 24);
>>
>> Why 'width - 24'?  Can it be expressed by a helper or macro?
> 
>                 /*
>                  * The allowed width are 24bit and 32bit, as they are
> supported by
>                  * aud2htx module.
>                  * for 24bit, D_SEL = 0, select all the bits.
>                  * for 32bit, D_SEL = 8, select the MSB.
>                  */
> will add such comments.

Fine for me(Not an audio guy though).

-- 
Regards,
Liu Ying

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Alexander Stein]

Hi,

Am Mittwoch, 6. August 2025, 05:49:13 CEST schrieb Shengjiu Wang:
> On Tue, Aug 5, 2025 at 3:09 PM Alexander Stein
> <alexander.stein@ew.tq-group.com> wrote:
> [snip]
> > > +static int imx8mp_dw_hdmi_bind(struct device *dev)
> > > +{
> > > +     struct dw_hdmi_plat_data *plat_data;
> > > +     struct imx8mp_hdmi *hdmi;
> > > +     int ret;
> > > +
> > > +     hdmi = dev_get_drvdata(dev);
> > > +     plat_data = &hdmi->plat_data;
> > > +
> > > +     ret = component_bind_all(dev, plat_data);
> >
> > Do you really need plat_data variable?
> 
> yes,  it is used in imx8mp_hdmi_pai_bind()

Sorry for not being clear. I'm not talking about struct dw_hdmi_plat_data, but
the local variable plat_data. You can use

ret = component_bind_all(dev, &hdmi->plat_data);

directly.

> 
> >
> > > +     if (ret)
> > > +             return dev_err_probe(dev, ret, "component_bind_all failed!\n");
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static void imx8mp_dw_hdmi_unbind(struct device *dev)
> > > +{
> > > +     struct dw_hdmi_plat_data *plat_data;
> > > +     struct imx8mp_hdmi *hdmi;
> > > +
> > > +     hdmi = dev_get_drvdata(dev);
> > > +     plat_data = &hdmi->plat_data;
> > > +
> > > +     component_unbind_all(dev, plat_data);
> >
> > Do you really need plat_data variable?
> 
> yes,  it is used by imx8mp_hdmi_pai_unbind()

Same as above. Call

component_unbind_all(dev, &hdmi->plat_data)

directly. Also consider assigning struct imx8mp_hdmi *hdmi = dev_get_drvdata(dev);
directly.

Best regards,
Alexander

> 
> >
> > > +}
> > > +
> > > +static const struct component_master_ops imx8mp_dw_hdmi_ops = {
> > > +     .bind   = imx8mp_dw_hdmi_bind,
> > > +     .unbind = imx8mp_dw_hdmi_unbind,
> > > +};
> > > +
> > >  static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
> > >  {
> > >       struct device *dev = &pdev->dev;
> > >       struct dw_hdmi_plat_data *plat_data;
> > > +     struct component_match *match;
> >
> > Set match = NULL for drm_of_component_match_add (and subcalls) to allocate memory.
> 
> Ok.
> 
> best regards
> Shengjiu wang.
> >
> > Best regards
> > Alexander
> >
> > > +     struct device_node *remote;
> > >       struct imx8mp_hdmi *hdmi;
> > > +     int ret;
> > >
> > >       hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
> > >       if (!hdmi)
> > > @@ -108,6 +145,22 @@ static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
> > >
> > >       platform_set_drvdata(pdev, hdmi);
> > >
> > > +     /* port@2 is for hdmi_pai device */
> > > +     remote = of_graph_get_remote_node(pdev->dev.of_node, 2, 0);
> > > +     if (remote && of_device_is_available(remote)) {
> > > +             drm_of_component_match_add(dev, &match, component_compare_of, remote);
> > > +
> > > +             of_node_put(remote);
> > > +
> > > +             ret = component_master_add_with_match(dev, &imx8mp_dw_hdmi_ops, match);
> > > +             if (ret)
> > > +                     dev_warn(dev, "Unable to register aggregate driver\n");
> > > +             /*
> > > +              * This audio function is optional for avoid blocking display.
> > > +              * So just print warning message and no error is returned.
> > > +              */
> > > +     }
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -115,6 +168,8 @@ static void imx8mp_dw_hdmi_remove(struct platform_device *pdev)
> > >  {
> > >       struct imx8mp_hdmi *hdmi = platform_get_drvdata(pdev);
> > >
> > > +     component_master_del(&pdev->dev, &imx8mp_dw_hdmi_ops);
> > > +
> > >       dw_hdmi_remove(hdmi->dw_hdmi);
> > >  }
> > >
> > > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> > > index 095cdd9b7424..336f062e1f9d 100644
> > > --- a/include/drm/bridge/dw_hdmi.h
> > > +++ b/include/drm/bridge/dw_hdmi.h
> > > @@ -143,6 +143,12 @@ struct dw_hdmi_plat_data {
> > >                                          const struct drm_display_info *info,
> > >                                          const struct drm_display_mode *mode);
> > >
> > > +     /*
> > > +      * priv_audio is specially used for additional audio device to get
> > > +      * driver data through this dw_hdmi_plat_data.
> > > +      */
> > > +     void *priv_audio;
> > > +
> > >       /* Platform-specific audio enable/disable (optional) */
> > >       void (*enable_audio)(struct dw_hdmi *hdmi, int channel,
> > >                            int width, int rate, int non_pcm, int iec958);
> > >
> >
> >
> > --
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > http://www.tq-group.com/
> >
> >
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Shengjiu Wang]

On Thu, Aug 7, 2025 at 2:48 PM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi,
>
> Am Mittwoch, 6. August 2025, 05:49:13 CEST schrieb Shengjiu Wang:
> > On Tue, Aug 5, 2025 at 3:09 PM Alexander Stein
> > <alexander.stein@ew.tq-group.com> wrote:
> > [snip]
> > > > +static int imx8mp_dw_hdmi_bind(struct device *dev)
> > > > +{
> > > > +     struct dw_hdmi_plat_data *plat_data;
> > > > +     struct imx8mp_hdmi *hdmi;
> > > > +     int ret;
> > > > +
> > > > +     hdmi = dev_get_drvdata(dev);
> > > > +     plat_data = &hdmi->plat_data;
> > > > +
> > > > +     ret = component_bind_all(dev, plat_data);
> > >
> > > Do you really need plat_data variable?
> >
> > yes,  it is used in imx8mp_hdmi_pai_bind()
>
> Sorry for not being clear. I'm not talking about struct dw_hdmi_plat_data, but
> the local variable plat_data. You can use
>
> ret = component_bind_all(dev, &hdmi->plat_data);
>
> directly.

Ok,  will update the code.

>
> >
> > >
> > > > +     if (ret)
> > > > +             return dev_err_probe(dev, ret, "component_bind_all failed!\n");
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static void imx8mp_dw_hdmi_unbind(struct device *dev)
> > > > +{
> > > > +     struct dw_hdmi_plat_data *plat_data;
> > > > +     struct imx8mp_hdmi *hdmi;
> > > > +
> > > > +     hdmi = dev_get_drvdata(dev);
> > > > +     plat_data = &hdmi->plat_data;
> > > > +
> > > > +     component_unbind_all(dev, plat_data);
> > >
> > > Do you really need plat_data variable?
> >
> > yes,  it is used by imx8mp_hdmi_pai_unbind()
>
> Same as above. Call
>
> component_unbind_all(dev, &hdmi->plat_data)
>
> directly. Also consider assigning struct imx8mp_hdmi *hdmi = dev_get_drvdata(dev);
> directly.

Ok,  will update the code. Thanks.

Best regards
Shengjiu Wang
>
> Best regards,
> Alexander
>
> >
> > >
> > > > +}
> > > > +
> > > > +static const struct component_master_ops imx8mp_dw_hdmi_ops = {
> > > > +     .bind   = imx8mp_dw_hdmi_bind,
> > > > +     .unbind = imx8mp_dw_hdmi_unbind,
> > > > +};
> > > > +
> > > >  static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
> > > >  {
> > > >       struct device *dev = &pdev->dev;
> > > >       struct dw_hdmi_plat_data *plat_data;
> > > > +     struct component_match *match;
> > >
> > > Set match = NULL for drm_of_component_match_add (and subcalls) to allocate memory.
> >
> > Ok.
> >
> > best regards
> > Shengjiu wang.
> > >
> > > Best regards
> > > Alexander
> > >
> > > > +     struct device_node *remote;
> > > >       struct imx8mp_hdmi *hdmi;
> > > > +     int ret;
> > > >
> > > >       hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
> > > >       if (!hdmi)
> > > > @@ -108,6 +145,22 @@ static int imx8mp_dw_hdmi_probe(struct platform_device *pdev)
> > > >
> > > >       platform_set_drvdata(pdev, hdmi);
> > > >
> > > > +     /* port@2 is for hdmi_pai device */
> > > > +     remote = of_graph_get_remote_node(pdev->dev.of_node, 2, 0);
> > > > +     if (remote && of_device_is_available(remote)) {
> > > > +             drm_of_component_match_add(dev, &match, component_compare_of, remote);
> > > > +
> > > > +             of_node_put(remote);
> > > > +
> > > > +             ret = component_master_add_with_match(dev, &imx8mp_dw_hdmi_ops, match);
> > > > +             if (ret)
> > > > +                     dev_warn(dev, "Unable to register aggregate driver\n");
> > > > +             /*
> > > > +              * This audio function is optional for avoid blocking display.
> > > > +              * So just print warning message and no error is returned.
> > > > +              */
> > > > +     }
> > > > +
> > > >       return 0;
> > > >  }
> > > >
> > > > @@ -115,6 +168,8 @@ static void imx8mp_dw_hdmi_remove(struct platform_device *pdev)
> > > >  {
> > > >       struct imx8mp_hdmi *hdmi = platform_get_drvdata(pdev);
> > > >
> > > > +     component_master_del(&pdev->dev, &imx8mp_dw_hdmi_ops);
> > > > +
> > > >       dw_hdmi_remove(hdmi->dw_hdmi);
> > > >  }
> > > >
> > > > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> > > > index 095cdd9b7424..336f062e1f9d 100644
> > > > --- a/include/drm/bridge/dw_hdmi.h
> > > > +++ b/include/drm/bridge/dw_hdmi.h
> > > > @@ -143,6 +143,12 @@ struct dw_hdmi_plat_data {
> > > >                                          const struct drm_display_info *info,
> > > >                                          const struct drm_display_mode *mode);
> > > >
> > > > +     /*
> > > > +      * priv_audio is specially used for additional audio device to get
> > > > +      * driver data through this dw_hdmi_plat_data.
> > > > +      */
> > > > +     void *priv_audio;
> > > > +
> > > >       /* Platform-specific audio enable/disable (optional) */
> > > >       void (*enable_audio)(struct dw_hdmi *hdmi, int channel,
> > > >                            int width, int rate, int non_pcm, int iec958);
> > > >
> > >
> > >
> > > --
> > > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > > Amtsgericht München, HRB 105018
> > > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > > http://www.tq-group.com/
> > >
> > >
> >
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/
>
>

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Shengjiu Wang]

On Wed, Aug 6, 2025 at 2:52 PM Liu Ying <victor.liu@nxp.com> wrote:
>
> On 08/06/2025, Shengjiu Wang wrote:
> > On Tue, Aug 5, 2025 at 4:55 PM Liu Ying <victor.liu@nxp.com> wrote:
> >>
> >> On 08/04/2025, Shengjiu Wang wrote:
>
> [...]
>
> >>> +static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
> >>> +{
> >>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> >>> +     struct imx8mp_hdmi_pai *hdmi_pai;
> >>> +
> >>> +     hdmi_pai = dev_get_drvdata(dev);
> >>> +
> >>> +     plat_data->enable_audio = imx8mp_hdmi_pai_enable;
> >>> +     plat_data->disable_audio = imx8mp_hdmi_pai_disable;
> >>> +     plat_data->priv_audio = hdmi_pai;
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
> >>> +{
> >>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> >>> +
> >>> +     plat_data->enable_audio = NULL;
> >>> +     plat_data->disable_audio = NULL;
> >>> +     plat_data->priv_audio = NULL;
> >>
> >> Do you really need to set these ptrs to NULL?
> >
> > yes.  below code in dw-hdmi.c use the pdata->enable_audio as condition.
>
> Note that this is all about tearing down components.
> If this is done properly as the below snippet of pseudo-code, then
> hdmi->{enable,disable}_audio() and pdata->{enable,disable}_audio() won't be
> called after audio device is removed by dw_hdmi_remove().  So, it's unnecessary
> to set these pointers to NULL here.
>
> imx8mp_dw_hdmi_unbind()
> {
>    dw_hdmi_remove(); // platform_device_unregister(hdmi->audio);
>    component_unbind_all(); //imx8mp_hdmi_pai_unbind()
> }
>
> BTW, I suggest the below snippet[1] to bind components.
>
> imx8mp_dw_hdmi_bind()
> {
>    component_bind_all(); // imx8mp_hdmi_pai_bind()
>                          //   set pdata->{enable,disable}_audio
>    dw_hdmi_probe(); // hdmi->audio = platform_device_register_full(&pdevinfo);
> }

Looks like we should use dw_hdmi_bind() here to make unbind -> bind work.
But can't get the encoder pointer.  the encoder pointer is from lcdif_drv.c,
the probe sequence of lcdif, pvi, dw_hdmi should be dw_hdmi first, then pvi,
then lcdif, because current implementation in lcdif and pvi driver.

Should the lcdif and pvi driver be modified to use component helper?
This seems out of the scope of this patch set.

Best regards
Shengjiu Wang
>
> >
> >         if (pdata->enable_audio)
> >                 pdata->enable_audio(hdmi,
> >                                     hdmi->channels,
> >                                     hdmi->sample_width,
> >                                     hdmi->sample_rate,
> >                                     hdmi->sample_non_pcm,
> >                                     hdmi->sample_iec958);
> >
> >
> >>
>
> [...]
>
> >>> +     return component_add(dev, &imx8mp_hdmi_pai_ops);
> >>
> >> Imagine that users could enable this driver without enabling imx8mp-hdmi-tx
> >> driver, you may add the component in this probe() callback only and move all
> >> the other stuff to bind() callback to avoid unnecessary things being done here.
> >
> > component helper functions don't have such dependency that the aggregate
> > driver or component driver must be probed or not.  if imx8mp-hdmi-tx is not
> > enabled, there is no problem, just the bind() callback is not called.
>
> I meant I'd write imx8mp_hdmi_pai_probe() as below snippet and do all the
> other stuff in imx8mp_hdmi_pai_bind().  This ensures minimum things are done
> in imx8mp_hdmi_pai_probe() if imx8mp-hdmi-tx doesn't probe.
>
> static int imx8mp_hdmi_pai_probe(struct platform_device *pdev)
> {
>         return component_add(&pdev->dev, &imx8mp_hdmi_pai_ops);
> }
>
> >
> >>
> >>> +}
> >>> +
> >>> +static void imx8mp_hdmi_pai_remove(struct platform_device *pdev)
> >>> +{
> >>> +     component_del(&pdev->dev, &imx8mp_hdmi_pai_ops);
> >>> +}
> >>> +
> >>> +static const struct of_device_id imx8mp_hdmi_pai_of_table[] = {
> >>> +     { .compatible = "fsl,imx8mp-hdmi-pai" },
> >>> +     { /* Sentinel */ }
> >>> +};
> >>> +MODULE_DEVICE_TABLE(of, imx8mp_hdmi_pai_of_table);
> >>> +
> >>> +static struct platform_driver imx8mp_hdmi_pai_platform_driver = {
> >>> +     .probe          = imx8mp_hdmi_pai_probe,
> >>> +     .remove         = imx8mp_hdmi_pai_remove,
> >>> +     .driver         = {
> >>> +             .name   = "imx8mp-hdmi-pai",
> >>> +             .of_match_table = imx8mp_hdmi_pai_of_table,
> >>> +     },
> >>> +};
> >>> +module_platform_driver(imx8mp_hdmi_pai_platform_driver);
> >>> +
> >>> +MODULE_DESCRIPTION("i.MX8MP HDMI PAI driver");
> >>> +MODULE_LICENSE("GPL");
> >>> diff --git a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> >>> index 1e7a789ec289..ee08084d2394 100644
> >>> --- a/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> >>> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> >>> @@ -5,11 +5,13 @@
> >>>   */
> >>>
> >>>  #include <linux/clk.h>
> >>> +#include <linux/component.h>
> >>>  #include <linux/mod_devicetable.h>
> >>>  #include <linux/module.h>
> >>>  #include <linux/platform_device.h>
> >>>  #include <drm/bridge/dw_hdmi.h>
> >>>  #include <drm/drm_modes.h>
> >>> +#include <drm/drm_of.h>
> >>>
> >>>  struct imx8mp_hdmi {
> >>>       struct dw_hdmi_plat_data plat_data;
> >>> @@ -79,11 +81,46 @@ static const struct dw_hdmi_phy_ops imx8mp_hdmi_phy_ops = {
> >>>       .update_hpd     = dw_hdmi_phy_update_hpd,
> >>>  };
> >>>
> >>> +static int imx8mp_dw_hdmi_bind(struct device *dev)
> >>> +{
> >>> +     struct dw_hdmi_plat_data *plat_data;
> >>> +     struct imx8mp_hdmi *hdmi;
> >>> +     int ret;
> >>> +
> >>> +     hdmi = dev_get_drvdata(dev);
> >>> +     plat_data = &hdmi->plat_data;
> >>> +
> >>> +     ret = component_bind_all(dev, plat_data);
> >>> +     if (ret)
> >>> +             return dev_err_probe(dev, ret, "component_bind_all failed!\n");
> >>
> >> As component_bind_all() would bind imx8mp-hdmi-pai and hence set
> >> {enable,disable}_audio callbacks, you need to call dw_hdmi_probe() after
> >> component_bind_all() instead of too early in probe() callback.
> >
> > There is no such dependency.
> > Maybe you mixed the hdmi->enable_audio() with pdata->enable_audio().
>
> As the above snippet[1] shows, once dw_hdmi_probe() registers audio device,
> the audio device could be functional soon after audio driver probes, hence
> hdmi->enable_audio() would be called and hence pdata->enable_audio() would
> be called. So, you need to set pdata->enable_audio() before dw_hdmi_probe()
> is called, otherwise pdata->enable_audio could be NULL when is called by
> audio driver.
>
> [...]
>
> >>> +     remote = of_graph_get_remote_node(pdev->dev.of_node, 2, 0);
> >>> +     if (remote && of_device_is_available(remote)) {
> >>
> >> Doesn't of_graph_get_remote_node() ensure that remote is avaiable?
> >
> > No.  'remote' is the node,  not the 'device'.
>
> See of_device_is_available() is called by of_graph_get_remote_node():
>
> struct device_node *of_graph_get_remote_node(const struct device_node *node,
>                                              u32 port, u32 endpoint)
> {
>         struct device_node *endpoint_node, *remote;
>
>         endpoint_node = of_graph_get_endpoint_by_regs(node, port, endpoint);
>         if (!endpoint_node) {
>                 pr_debug("no valid endpoint (%d, %d) for node %pOF\n",
>                          port, endpoint, node);
>                 return NULL;
>         }
>
>         remote = of_graph_get_remote_port_parent(endpoint_node);
>         of_node_put(endpoint_node);
>         if (!remote) {
>                 pr_debug("no valid remote node\n");
>                 return NULL;
>         }
>
>         if (!of_device_is_available(remote)) {
>              ^~~~~~~~~~~~~~~~~~~~~~
>                 pr_debug("not available for remote node\n");
>                 of_node_put(remote);
>                 return NULL;
>         }
>
>         return remote;
> }
> EXPORT_SYMBOL(of_graph_get_remote_node);
>
> >
> >>
> >>> +             drm_of_component_match_add(dev, &match, component_compare_of, remote);
> >>> +
> >>> +             of_node_put(remote);
> >>> +
> >>> +             ret = component_master_add_with_match(dev, &imx8mp_dw_hdmi_ops, match);
> >>> +             if (ret)
> >>> +                     dev_warn(dev, "Unable to register aggregate driver\n");
> >>> +             /*
> >>> +              * This audio function is optional for avoid blocking display.
> >>> +              * So just print warning message and no error is returned.
> >>
> >> No, since PAI node is available here, it has to be bound.  Yet you still need
> >> to properly handle the case where PAI node is inavailable.
> >
> > This is for aggregate driver registration,  not for bind()
> >
> > The bind() is called after both drivers have been registered.  again there is no
> > dependency for both aggregate driver and component driver should be
> > registered or probed.
>
> Sorry for not being clear about my previous wording.  I meant since PAI node is
> available here, component_master_add_with_match() must be called to register
> the master and if it fails to register it, imx8mp_dw_hdmi_probe() should return
> proper error code, not 0.
>
> --
> Regards,
> Liu Ying

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Liu Ying]

On 08/07/2025, Shengjiu Wang wrote:
> On Wed, Aug 6, 2025 at 2:52 PM Liu Ying <victor.liu@nxp.com> wrote:
>>
>> On 08/06/2025, Shengjiu Wang wrote:
>>> On Tue, Aug 5, 2025 at 4:55 PM Liu Ying <victor.liu@nxp.com> wrote:
>>>>
>>>> On 08/04/2025, Shengjiu Wang wrote:
>>
>> [...]
>>
>>>>> +static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
>>>>> +{
>>>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
>>>>> +     struct imx8mp_hdmi_pai *hdmi_pai;
>>>>> +
>>>>> +     hdmi_pai = dev_get_drvdata(dev);
>>>>> +
>>>>> +     plat_data->enable_audio = imx8mp_hdmi_pai_enable;
>>>>> +     plat_data->disable_audio = imx8mp_hdmi_pai_disable;
>>>>> +     plat_data->priv_audio = hdmi_pai;
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
>>>>> +{
>>>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
>>>>> +
>>>>> +     plat_data->enable_audio = NULL;
>>>>> +     plat_data->disable_audio = NULL;
>>>>> +     plat_data->priv_audio = NULL;
>>>>
>>>> Do you really need to set these ptrs to NULL?
>>>
>>> yes.  below code in dw-hdmi.c use the pdata->enable_audio as condition.
>>
>> Note that this is all about tearing down components.
>> If this is done properly as the below snippet of pseudo-code, then
>> hdmi->{enable,disable}_audio() and pdata->{enable,disable}_audio() won't be
>> called after audio device is removed by dw_hdmi_remove().  So, it's unnecessary
>> to set these pointers to NULL here.
>>
>> imx8mp_dw_hdmi_unbind()
>> {
>>    dw_hdmi_remove(); // platform_device_unregister(hdmi->audio);
>>    component_unbind_all(); //imx8mp_hdmi_pai_unbind()
>> }
>>
>> BTW, I suggest the below snippet[1] to bind components.
>>
>> imx8mp_dw_hdmi_bind()
>> {
>>    component_bind_all(); // imx8mp_hdmi_pai_bind()
>>                          //   set pdata->{enable,disable}_audio
>>    dw_hdmi_probe(); // hdmi->audio = platform_device_register_full(&pdevinfo);
>> }
> 
> Looks like we should use dw_hdmi_bind() here to make unbind -> bind work.

I don't get your idea here.

What are you trying to make work?
Why dw_hdmi_probe() can't be used?
How does dw_hdmi_bind() help here?

> But can't get the encoder pointer.  the encoder pointer is from lcdif_drv.c,
> the probe sequence of lcdif, pvi, dw_hdmi should be dw_hdmi first, then pvi,
> then lcdif, because current implementation in lcdif and pvi driver.

We use deferral probe to make sure the probe sequence is
DW_HDMI -> PVI -> LCDIF.

LCDIF driver would call devm_drm_of_get_bridge() to get the next bridge PVI
and it defers probe if devm_drm_of_get_bridge() returns ERR_PTR(-EPROBE_DEFER).
Same to PVI driver, it would call of_drm_find_bridge() to get the next bridge
DW_HDMI and defers probe if needed.

> 
> Should the lcdif and pvi driver be modified to use component helper?

Why should they use component helper?

BTW, I've tried testing the snippets suggested by me on i.MX8MP EVK and
the components bind successfully:

cat /sys/kernel/debug/device_component/32fd8000.hdmi
aggregate_device name                                            status
-----------------------------------------------------------------------
32fd8000.hdmi                                                     bound

device name                                                      status
-----------------------------------------------------------------------
32fc4800.audio-bridge                                             bound

> This seems out of the scope of this patch set.
> 
> Best regards
> Shengjiu Wang

[...]

-- 
Regards,
Liu Ying

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Shengjiu Wang]

On Fri, Aug 8, 2025 at 2:32 PM Liu Ying <victor.liu@nxp.com> wrote:
>
> On 08/07/2025, Shengjiu Wang wrote:
> > On Wed, Aug 6, 2025 at 2:52 PM Liu Ying <victor.liu@nxp.com> wrote:
> >>
> >> On 08/06/2025, Shengjiu Wang wrote:
> >>> On Tue, Aug 5, 2025 at 4:55 PM Liu Ying <victor.liu@nxp.com> wrote:
> >>>>
> >>>> On 08/04/2025, Shengjiu Wang wrote:
> >>
> >> [...]
> >>
> >>>>> +static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
> >>>>> +{
> >>>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> >>>>> +     struct imx8mp_hdmi_pai *hdmi_pai;
> >>>>> +
> >>>>> +     hdmi_pai = dev_get_drvdata(dev);
> >>>>> +
> >>>>> +     plat_data->enable_audio = imx8mp_hdmi_pai_enable;
> >>>>> +     plat_data->disable_audio = imx8mp_hdmi_pai_disable;
> >>>>> +     plat_data->priv_audio = hdmi_pai;
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
> >>>>> +{
> >>>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> >>>>> +
> >>>>> +     plat_data->enable_audio = NULL;
> >>>>> +     plat_data->disable_audio = NULL;
> >>>>> +     plat_data->priv_audio = NULL;
> >>>>
> >>>> Do you really need to set these ptrs to NULL?
> >>>
> >>> yes.  below code in dw-hdmi.c use the pdata->enable_audio as condition.
> >>
> >> Note that this is all about tearing down components.
> >> If this is done properly as the below snippet of pseudo-code, then
> >> hdmi->{enable,disable}_audio() and pdata->{enable,disable}_audio() won't be
> >> called after audio device is removed by dw_hdmi_remove().  So, it's unnecessary
> >> to set these pointers to NULL here.
> >>
> >> imx8mp_dw_hdmi_unbind()
> >> {
> >>    dw_hdmi_remove(); // platform_device_unregister(hdmi->audio);
> >>    component_unbind_all(); //imx8mp_hdmi_pai_unbind()
> >> }
> >>
> >> BTW, I suggest the below snippet[1] to bind components.
> >>
> >> imx8mp_dw_hdmi_bind()
> >> {
> >>    component_bind_all(); // imx8mp_hdmi_pai_bind()
> >>                          //   set pdata->{enable,disable}_audio
> >>    dw_hdmi_probe(); // hdmi->audio = platform_device_register_full(&pdevinfo);
> >> }
> >
> > Looks like we should use dw_hdmi_bind() here to make unbind -> bind work.
>
> I don't get your idea here.
>
> What are you trying to make work?
> Why dw_hdmi_probe() can't be used?
> How does dw_hdmi_bind() help here?

bind() is ok.  but unbind(),  then bind() there is an issue.

>
> > But can't get the encoder pointer.  the encoder pointer is from lcdif_drv.c,
> > the probe sequence of lcdif, pvi, dw_hdmi should be dw_hdmi first, then pvi,
> > then lcdif, because current implementation in lcdif and pvi driver.
>
> We use deferral probe to make sure the probe sequence is
> DW_HDMI -> PVI -> LCDIF.
>
> LCDIF driver would call devm_drm_of_get_bridge() to get the next bridge PVI
> and it defers probe if devm_drm_of_get_bridge() returns ERR_PTR(-EPROBE_DEFER).
> Same to PVI driver, it would call of_drm_find_bridge() to get the next bridge
> DW_HDMI and defers probe if needed.

right, probe is no problem,  but after probe,  if unbind pai, hdmi_tx,
 then bind
them again,  there is a problem,  because no one call the
drm_bridge_attach() again.

>
> >
> > Should the lcdif and pvi driver be modified to use component helper?
>
> Why should they use component helper?
>
> BTW, I've tried testing the snippets suggested by me on i.MX8MP EVK and
> the components bind successfully:

right, probe is no problem. but if try to unbind() then bind, there is issue.

best regards
shengjiu Wang
>
> cat /sys/kernel/debug/device_component/32fd8000.hdmi
> aggregate_device name                                            status
> -----------------------------------------------------------------------
> 32fd8000.hdmi                                                     bound
>
> device name                                                      status
> -----------------------------------------------------------------------
> 32fc4800.audio-bridge                                             bound
>
> > This seems out of the scope of this patch set.
> >
> > Best regards
> > Shengjiu Wang
>
> [...]
>
> --
> Regards,
> Liu Ying

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Liu Ying]

On 08/08/2025, Shengjiu Wang wrote:
> On Fri, Aug 8, 2025 at 2:32 PM Liu Ying <victor.liu@nxp.com> wrote:
>>
>> On 08/07/2025, Shengjiu Wang wrote:
>>> On Wed, Aug 6, 2025 at 2:52 PM Liu Ying <victor.liu@nxp.com> wrote:
>>>>
>>>> On 08/06/2025, Shengjiu Wang wrote:
>>>>> On Tue, Aug 5, 2025 at 4:55 PM Liu Ying <victor.liu@nxp.com> wrote:
>>>>>>
>>>>>> On 08/04/2025, Shengjiu Wang wrote:
>>>>
>>>> [...]
>>>>
>>>>>>> +static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
>>>>>>> +{
>>>>>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
>>>>>>> +     struct imx8mp_hdmi_pai *hdmi_pai;
>>>>>>> +
>>>>>>> +     hdmi_pai = dev_get_drvdata(dev);
>>>>>>> +
>>>>>>> +     plat_data->enable_audio = imx8mp_hdmi_pai_enable;
>>>>>>> +     plat_data->disable_audio = imx8mp_hdmi_pai_disable;
>>>>>>> +     plat_data->priv_audio = hdmi_pai;
>>>>>>> +
>>>>>>> +     return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
>>>>>>> +{
>>>>>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
>>>>>>> +
>>>>>>> +     plat_data->enable_audio = NULL;
>>>>>>> +     plat_data->disable_audio = NULL;
>>>>>>> +     plat_data->priv_audio = NULL;
>>>>>>
>>>>>> Do you really need to set these ptrs to NULL?
>>>>>
>>>>> yes.  below code in dw-hdmi.c use the pdata->enable_audio as condition.
>>>>
>>>> Note that this is all about tearing down components.
>>>> If this is done properly as the below snippet of pseudo-code, then
>>>> hdmi->{enable,disable}_audio() and pdata->{enable,disable}_audio() won't be
>>>> called after audio device is removed by dw_hdmi_remove().  So, it's unnecessary
>>>> to set these pointers to NULL here.
>>>>
>>>> imx8mp_dw_hdmi_unbind()
>>>> {
>>>>    dw_hdmi_remove(); // platform_device_unregister(hdmi->audio);
>>>>    component_unbind_all(); //imx8mp_hdmi_pai_unbind()
>>>> }
>>>>
>>>> BTW, I suggest the below snippet[1] to bind components.
>>>>
>>>> imx8mp_dw_hdmi_bind()
>>>> {
>>>>    component_bind_all(); // imx8mp_hdmi_pai_bind()
>>>>                          //   set pdata->{enable,disable}_audio
>>>>    dw_hdmi_probe(); // hdmi->audio = platform_device_register_full(&pdevinfo);
>>>> }
>>>
>>> Looks like we should use dw_hdmi_bind() here to make unbind -> bind work.
>>
>> I don't get your idea here.
>>
>> What are you trying to make work?
>> Why dw_hdmi_probe() can't be used?
>> How does dw_hdmi_bind() help here?
> 
> bind() is ok.  but unbind(),  then bind() there is an issue.
> 
>>
>>> But can't get the encoder pointer.  the encoder pointer is from lcdif_drv.c,
>>> the probe sequence of lcdif, pvi, dw_hdmi should be dw_hdmi first, then pvi,
>>> then lcdif, because current implementation in lcdif and pvi driver.
>>
>> We use deferral probe to make sure the probe sequence is
>> DW_HDMI -> PVI -> LCDIF.
>>
>> LCDIF driver would call devm_drm_of_get_bridge() to get the next bridge PVI
>> and it defers probe if devm_drm_of_get_bridge() returns ERR_PTR(-EPROBE_DEFER).
>> Same to PVI driver, it would call of_drm_find_bridge() to get the next bridge
>> DW_HDMI and defers probe if needed.
> 
> right, probe is no problem,  but after probe,  if unbind pai, hdmi_tx,
>  then bind
> them again,  there is a problem,  because no one call the
> drm_bridge_attach() again.

In my mind, this is a common issue as DRM bridges are not properly detached
and attached again.
For now, only drm_encoder_cleanup() calls drm_bridge_detach().

Anyway, this issue is not introduced by this patch series, i.e. it's already
there.

> 
>>
>>>
>>> Should the lcdif and pvi driver be modified to use component helper?
>>
>> Why should they use component helper?
>>
>> BTW, I've tried testing the snippets suggested by me on i.MX8MP EVK and
>> the components bind successfully:
> 
> right, probe is no problem. but if try to unbind() then bind, there is issue.

I don't think the DRM bridge detach/attach issue would be addressed by
using component helper.

> 
> best regards
> shengjiu Wang
>>
>> cat /sys/kernel/debug/device_component/32fd8000.hdmi
>> aggregate_device name                                            status
>> -----------------------------------------------------------------------
>> 32fd8000.hdmi                                                     bound
>>
>> device name                                                      status
>> -----------------------------------------------------------------------
>> 32fc4800.audio-bridge                                             bound
>>
>>> This seems out of the scope of this patch set.
>>>
>>> Best regards
>>> Shengjiu Wang
>>
>> [...]
>>
>> --
>> Regards,
>> Liu Ying


-- 
Regards,
Liu Ying

Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Shengjiu Wang]

On Fri, Aug 8, 2025 at 3:49 PM Liu Ying <victor.liu@nxp.com> wrote:
>
> On 08/08/2025, Shengjiu Wang wrote:
> > On Fri, Aug 8, 2025 at 2:32 PM Liu Ying <victor.liu@nxp.com> wrote:
> >>
> >> On 08/07/2025, Shengjiu Wang wrote:
> >>> On Wed, Aug 6, 2025 at 2:52 PM Liu Ying <victor.liu@nxp.com> wrote:
> >>>>
> >>>> On 08/06/2025, Shengjiu Wang wrote:
> >>>>> On Tue, Aug 5, 2025 at 4:55 PM Liu Ying <victor.liu@nxp.com> wrote:
> >>>>>>
> >>>>>> On 08/04/2025, Shengjiu Wang wrote:
> >>>>
> >>>> [...]
> >>>>
> >>>>>>> +static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
> >>>>>>> +{
> >>>>>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> >>>>>>> +     struct imx8mp_hdmi_pai *hdmi_pai;
> >>>>>>> +
> >>>>>>> +     hdmi_pai = dev_get_drvdata(dev);
> >>>>>>> +
> >>>>>>> +     plat_data->enable_audio = imx8mp_hdmi_pai_enable;
> >>>>>>> +     plat_data->disable_audio = imx8mp_hdmi_pai_disable;
> >>>>>>> +     plat_data->priv_audio = hdmi_pai;
> >>>>>>> +
> >>>>>>> +     return 0;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
> >>>>>>> +{
> >>>>>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
> >>>>>>> +
> >>>>>>> +     plat_data->enable_audio = NULL;
> >>>>>>> +     plat_data->disable_audio = NULL;
> >>>>>>> +     plat_data->priv_audio = NULL;
> >>>>>>
> >>>>>> Do you really need to set these ptrs to NULL?
> >>>>>
> >>>>> yes.  below code in dw-hdmi.c use the pdata->enable_audio as condition.
> >>>>
> >>>> Note that this is all about tearing down components.
> >>>> If this is done properly as the below snippet of pseudo-code, then
> >>>> hdmi->{enable,disable}_audio() and pdata->{enable,disable}_audio() won't be
> >>>> called after audio device is removed by dw_hdmi_remove().  So, it's unnecessary
> >>>> to set these pointers to NULL here.
> >>>>
> >>>> imx8mp_dw_hdmi_unbind()
> >>>> {
> >>>>    dw_hdmi_remove(); // platform_device_unregister(hdmi->audio);
> >>>>    component_unbind_all(); //imx8mp_hdmi_pai_unbind()
> >>>> }
> >>>>
> >>>> BTW, I suggest the below snippet[1] to bind components.
> >>>>
> >>>> imx8mp_dw_hdmi_bind()
> >>>> {
> >>>>    component_bind_all(); // imx8mp_hdmi_pai_bind()
> >>>>                          //   set pdata->{enable,disable}_audio
> >>>>    dw_hdmi_probe(); // hdmi->audio = platform_device_register_full(&pdevinfo);
> >>>> }
> >>>
> >>> Looks like we should use dw_hdmi_bind() here to make unbind -> bind work.
> >>
> >> I don't get your idea here.
> >>
> >> What are you trying to make work?
> >> Why dw_hdmi_probe() can't be used?
> >> How does dw_hdmi_bind() help here?
> >
> > bind() is ok.  but unbind(),  then bind() there is an issue.
> >
> >>
> >>> But can't get the encoder pointer.  the encoder pointer is from lcdif_drv.c,
> >>> the probe sequence of lcdif, pvi, dw_hdmi should be dw_hdmi first, then pvi,
> >>> then lcdif, because current implementation in lcdif and pvi driver.
> >>
> >> We use deferral probe to make sure the probe sequence is
> >> DW_HDMI -> PVI -> LCDIF.
> >>
> >> LCDIF driver would call devm_drm_of_get_bridge() to get the next bridge PVI
> >> and it defers probe if devm_drm_of_get_bridge() returns ERR_PTR(-EPROBE_DEFER).
> >> Same to PVI driver, it would call of_drm_find_bridge() to get the next bridge
> >> DW_HDMI and defers probe if needed.
> >
> > right, probe is no problem,  but after probe,  if unbind pai, hdmi_tx,
> >  then bind
> > them again,  there is a problem,  because no one call the
> > drm_bridge_attach() again.
>
> In my mind, this is a common issue as DRM bridges are not properly detached
> and attached again.
> For now, only drm_encoder_cleanup() calls drm_bridge_detach().
>
> Anyway, this issue is not introduced by this patch series, i.e. it's already
> there.

Ok,  thanks for clarification.

best regards
shengjiu Wang
>
> >
> >>
> >>>
> >>> Should the lcdif and pvi driver be modified to use component helper?
> >>
> >> Why should they use component helper?
> >>
> >> BTW, I've tried testing the snippets suggested by me on i.MX8MP EVK and
> >> the components bind successfully:
> >
> > right, probe is no problem. but if try to unbind() then bind, there is issue.
>
> I don't think the DRM bridge detach/attach issue would be addressed by
> using component helper.
>
> >
> > best regards
> > shengjiu Wang
> >>
> >> cat /sys/kernel/debug/device_component/32fd8000.hdmi
> >> aggregate_device name                                            status
> >> -----------------------------------------------------------------------
> >> 32fd8000.hdmi                                                     bound
> >>
> >> device name                                                      status
> >> -----------------------------------------------------------------------
> >> 32fc4800.audio-bridge                                             bound
> >>
> >>> This seems out of the scope of this patch set.
> >>>
> >>> Best regards
> >>> Shengjiu Wang
> >>
> >> [...]
> >>
> >> --
> >> Regards,
> >> Liu Ying
>
>
> --
> Regards,
> Liu Ying