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.w...@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
