| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAA+D8APjsiMnmnaAsxdcLi5g0hHgGSpFCgAxo042RK4yD-DraQ@mail.gmail.com>
Date: Wed, 23 Jul 2025 15:45:25 +0800
From: Shengjiu Wang <shengjiu.wang@...il.com>
To: Liu Ying <victor.liu@....com>
Cc: Shengjiu Wang <shengjiu.wang@....com>, andrzej.hajda@...el.com,
neil.armstrong@...aro.org, rfoss@...nel.org,
Laurent.pinchart@...asonboard.com, jonas@...boo.se, jernej.skrabec@...il.com,
maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de,
airlied@...il.com, simona@...ll.ch, lumag@...nel.org, dianders@...omium.org,
cristian.ciocaltea@...labora.com, luca.ceresoli@...tlin.com,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
festevam@...il.com, imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
p.zabel@...gutronix.de, devicetree@...r.kernel.org, l.stach@...gutronix.de
Subject: Re: [PATCH 2/4] drm/bridge: imx: add driver for HDMI TX Parallel
Audio Interface
On Tue, Jul 22, 2025 at 4:47 PM Liu Ying <victor.liu@....com> wrote:
>
> 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@....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?
yes.
>
> > +
> > 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.
Ok, will update them.
>
> > +
> > +#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?
yes.
>
> > +
> > + /* 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.
yes, will use component helper. with component helper, we can assign
the {enable, disable}_audio in .bind() callback, not matter which driver
probe first.
best regards
Shengjiu Wang
>
> > + 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
Powered by blists - more mailing lists