Re: [PATCH 2/4] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Liu Ying <victor.liu@nxp.com>]

Hi Shengjiu,

On 07/18/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.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  drivers/gpu/drm/bridge/imx/Kconfig           |   7 +
>  drivers/gpu/drm/bridge/imx/Makefile          |   1 +
>  drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c | 134 +++++++++++++++++++
>  include/drm/bridge/dw_hdmi.h                 |   6 +
>  4 files changed, 148 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..d95fa84a8dcd 100644
> --- a/drivers/gpu/drm/bridge/imx/Kconfig
> +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> @@ -24,6 +24,13 @@ config DRM_IMX8MP_DW_HDMI_BRIDGE
>  	  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.

Should DRM_IMX8MP_DW_HDMI_BRIDGE imply DRM_IMX8MP_HDMI_PAI as it implies
DRM_IMX8MP_HDMI_PVI and PHY_FSL_SAMSUNG_HDMI_PHY?

> +
>  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..f09ee2622e57
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pai.c
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2025 NXP
> + */
> +
> +#include <drm/bridge/dw_hdmi.h>

Usually, linux/*.h header files come before drm/*.h header files.

> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +#define HTX_PAI_CTRL                   0x00
> +#define HTX_PAI_CTRL_EXT               0x04
> +#define HTX_PAI_FIELD_CTRL             0x08
> +#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 unused.  Drop.

> +
> +#define CTRL_ENABLE                    BIT(0)

Drop CTRL_ prefix.  Same for the other bits/fields.

Define this bit under register HTX_PAI_CTRL.  Same for bits/fields of
the other registers.

> +
> +#define CTRL_EXT_WTMK_HIGH_MASK                GENMASK(31, 24)
> +#define CTRL_EXT_WTMK_HIGH             (0x3 << 24)

Add a parameter for the macro and use FIELD_PREP.
Same for WTMK_LOW and NUM_CH.

#define WTMK_HIGH(n)	FIELD_PREP(WTMK_HIGH_MASK, (n))

> +#define CTRL_EXT_WTMK_LOW_MASK         GENMASK(23, 16)
> +#define CTRL_EXT_WTMK_LOW              (0x3 << 16)
> +#define CTRL_EXT_NUM_CH_MASK           GENMASK(10, 8)
> +#define CTRL_EXT_NUM_CH_SHIFT          8

This is not needed if FIELD_PREP is used.

> +
> +#define FIELD_CTRL_B_FILT              BIT(31)
> +#define FIELD_CTRL_PARITY_EN           BIT(30)
> +#define FIELD_CTRL_END_SEL             BIT(29)
> +#define FIELD_CTRL_PRE_SEL             GENMASK(28, 24)
> +#define FIELD_CTRL_PRE_SEL_SHIFT       24
> +#define FIELD_CTRL_D_SEL               GENMASK(23, 20)
> +#define FIELD_CTRL_D_SEL_SHIFT         20
> +#define FIELD_CTRL_V_SEL               GENMASK(19, 15)
> +#define FIELD_CTRL_V_SEL_SHIFT         15
> +#define FIELD_CTRL_U_SEL               GENMASK(14, 10)
> +#define FIELD_CTRL_U_SEL_SHIFT         10
> +#define FIELD_CTRL_C_SEL               GENMASK(9, 5)
> +#define FIELD_CTRL_C_SEL_SHIFT         5
> +#define FIELD_CTRL_P_SEL               GENMASK(4, 0)
> +#define FIELD_CTRL_P_SEL_SHIFT         0
> +
> +struct imx8mp_hdmi_pai {
> +	struct device	*dev;
> +	void __iomem	*base;
> +};
> +
> +static void imx8mp_hdmi_pai_enable(struct dw_hdmi *dw_hdmi, int channel,
> +				   int width, int rate, int non_pcm)
> +{
> +	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 */

/* PAI set control extended */

> +	val = CTRL_EXT_WTMK_HIGH | CTRL_EXT_WTMK_LOW;
> +	val |= ((channel - 1) << CTRL_EXT_NUM_CH_SHIFT);
> +	writel(val, hdmi_pai->base + HTX_PAI_CTRL_EXT);

Can you use regmap API?

> +
> +	/* IEC60958 format */
> +	val = 31 << FIELD_CTRL_P_SEL_SHIFT;
> +	val |= 30 << FIELD_CTRL_C_SEL_SHIFT;
> +	val |= 29 << FIELD_CTRL_U_SEL_SHIFT;
> +	val |= 28 << FIELD_CTRL_V_SEL_SHIFT;
> +	val |= 4 << FIELD_CTRL_D_SEL_SHIFT;
> +	val |= 0 << FIELD_CTRL_PRE_SEL_SHIFT;
> +

Nit: remove this blank line.

> +	writel(val, hdmi_pai->base + HTX_PAI_FIELD_CTRL);

Nit: add a blank line here.

> +	/* PAI start running */
> +	writel(CTRL_ENABLE, hdmi_pai->base + HTX_PAI_CTRL);
> +}
> +
> +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 */
> +	writel(0, hdmi_pai->base + HTX_PAI_CTRL);
> +}
> +
> +static int imx8mp_hdmi_pai_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct dw_hdmi_plat_data *plat_data;
> +	struct imx8mp_hdmi_pai *hdmi_pai;
> +	struct device_node *remote;
> +	struct platform_device *hdmi_tx;
> +	struct resource *res;
> +
> +	hdmi_pai = devm_kzalloc(dev, sizeof(*hdmi_pai), GFP_KERNEL);
> +	if (!hdmi_pai)
> +		return -ENOMEM;
> +
> +	hdmi_pai->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(hdmi_pai->base))
> +		return PTR_ERR(hdmi_pai->base);
> +
> +	hdmi_pai->dev = dev;
> +
> +	remote = of_graph_get_remote_node(pdev->dev.of_node, 0, -1);
> +	if (!remote)
> +		return -EINVAL;
> +
> +	hdmi_tx = of_find_device_by_node(remote);
> +	if (!hdmi_tx)
> +		return -EINVAL;
> +
> +	plat_data = platform_get_drvdata(hdmi_tx);
> +	plat_data->enable_audio = imx8mp_hdmi_pai_enable;
> +	plat_data->disable_audio = imx8mp_hdmi_pai_disable;

{enable,disable}_audio callbacks could be set too late...
You are trying to probe this driver after imx8mp_hdmi_tx is probed,
i.e., after dw_hdmi_probe() is called in imx8mp_dw_hdmi_probe().
Note that after dw_hdmi_probe() is called, the audio device could be
functional soon, while this probe is called asynchronously.

Also, what if imx8mp_hdmi_pai module is removed while imx8mp_hdmi_tx
is running?  Leaking {enable,disable}_audio callbacks?

I think that you may try to use component helper to take imx8mp_hdmi_tx
as an aggregate driver and this driver as a component driver.  After
the component is bound, you may set {enable,disable}_audio callbacks
in imx8mp_hdmi_tx before calling dw_hdmi_probe().
And, you need to export imx8mp_hdmi_pai_{enable,disable} symbols.

> +	plat_data->priv_audio = hdmi_pai;
> +
> +	return 0;
> +}
> +
> +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,
> +	.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/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index a56a3519a22a..9ca70ce80cc5 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);

-- 
Regards,
Liu Ying