[PATCH 1/2] drm/bridge: imx: fix mixed module-builtin object

[PATCH 1/2] drm/bridge: imx: fix mixed module-builtin object

[Masahiro Yamada]

With CONFIG_DRM_IMX8QM_LDB=m and CONFIG_DRM_IMX8QXP_LDB=y (or vice
versa), imx-ldb-helper.o is linked to a module and also to vmlinux
even though the expected CFLAGS are different between builtins and
modules.

This is the same situation as fixed by commit 637a642f5ca5 ("zstd:
Fixing mixed module-builtin objects").

Turn helpers in imx-ldb-helper.c into inline functions.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 drivers/gpu/drm/bridge/imx/Makefile         |   4 +-
 drivers/gpu/drm/bridge/imx/imx-ldb-helper.c | 221 --------------------
 drivers/gpu/drm/bridge/imx/imx-ldb-helper.h | 213 +++++++++++++++++--
 3 files changed, 197 insertions(+), 241 deletions(-)
 delete mode 100644 drivers/gpu/drm/bridge/imx/imx-ldb-helper.c

diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile
index aa90ec8d5433..64b93009376a 100644
--- a/drivers/gpu/drm/bridge/imx/Makefile
+++ b/drivers/gpu/drm/bridge/imx/Makefile
@@ -1,7 +1,7 @@
-imx8qm-ldb-objs := imx-ldb-helper.o imx8qm-ldb-drv.o
+imx8qm-ldb-objs := imx8qm-ldb-drv.o
 obj-$(CONFIG_DRM_IMX8QM_LDB) += imx8qm-ldb.o
 
-imx8qxp-ldb-objs := imx-ldb-helper.o imx8qxp-ldb-drv.o
+imx8qxp-ldb-objs := imx8qxp-ldb-drv.o
 obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
 
 obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o
diff --git a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
deleted file mode 100644
index 7338b84bc83d..000000000000
--- a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
+++ /dev/null
@@ -1,221 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * Copyright (C) 2012 Sascha Hauer, Pengutronix
- * Copyright 2019,2020,2022 NXP
- */
-
-#include <linux/media-bus-format.h>
-#include <linux/mfd/syscon.h>
-#include <linux/of.h>
-#include <linux/regmap.h>
-
-#include <drm/drm_bridge.h>
-#include <drm/drm_of.h>
-#include <drm/drm_print.h>
-
-#include "imx-ldb-helper.h"
-
-bool ldb_channel_is_single_link(struct ldb_channel *ldb_ch)
-{
-	return ldb_ch->link_type == LDB_CH_SINGLE_LINK;
-}
-
-bool ldb_channel_is_split_link(struct ldb_channel *ldb_ch)
-{
-	return ldb_ch->link_type == LDB_CH_DUAL_LINK_EVEN_ODD_PIXELS ||
-	       ldb_ch->link_type == LDB_CH_DUAL_LINK_ODD_EVEN_PIXELS;
-}
-
-int ldb_bridge_atomic_check_helper(struct drm_bridge *bridge,
-				   struct drm_bridge_state *bridge_state,
-				   struct drm_crtc_state *crtc_state,
-				   struct drm_connector_state *conn_state)
-{
-	struct ldb_channel *ldb_ch = bridge->driver_private;
-
-	ldb_ch->in_bus_format = bridge_state->input_bus_cfg.format;
-	ldb_ch->out_bus_format = bridge_state->output_bus_cfg.format;
-
-	return 0;
-}
-
-void ldb_bridge_mode_set_helper(struct drm_bridge *bridge,
-				const struct drm_display_mode *mode,
-				const struct drm_display_mode *adjusted_mode)
-{
-	struct ldb_channel *ldb_ch = bridge->driver_private;
-	struct ldb *ldb = ldb_ch->ldb;
-	bool is_split = ldb_channel_is_split_link(ldb_ch);
-
-	if (is_split)
-		ldb->ldb_ctrl |= LDB_SPLIT_MODE_EN;
-
-	switch (ldb_ch->out_bus_format) {
-	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
-		break;
-	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
-		if (ldb_ch->chno == 0 || is_split)
-			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24;
-		if (ldb_ch->chno == 1 || is_split)
-			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24;
-		break;
-	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
-		if (ldb_ch->chno == 0 || is_split)
-			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24 |
-					 LDB_BIT_MAP_CH0_JEIDA;
-		if (ldb_ch->chno == 1 || is_split)
-			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24 |
-					 LDB_BIT_MAP_CH1_JEIDA;
-		break;
-	}
-}
-
-void ldb_bridge_enable_helper(struct drm_bridge *bridge)
-{
-	struct ldb_channel *ldb_ch = bridge->driver_private;
-	struct ldb *ldb = ldb_ch->ldb;
-
-	/*
-	 * Platform specific bridge drivers should set ldb_ctrl properly
-	 * for the enablement, so just write the ctrl_reg here.
-	 */
-	regmap_write(ldb->regmap, ldb->ctrl_reg, ldb->ldb_ctrl);
-}
-
-void ldb_bridge_disable_helper(struct drm_bridge *bridge)
-{
-	struct ldb_channel *ldb_ch = bridge->driver_private;
-	struct ldb *ldb = ldb_ch->ldb;
-	bool is_split = ldb_channel_is_split_link(ldb_ch);
-
-	if (ldb_ch->chno == 0 || is_split)
-		ldb->ldb_ctrl &= ~LDB_CH0_MODE_EN_MASK;
-	if (ldb_ch->chno == 1 || is_split)
-		ldb->ldb_ctrl &= ~LDB_CH1_MODE_EN_MASK;
-
-	regmap_write(ldb->regmap, ldb->ctrl_reg, ldb->ldb_ctrl);
-}
-
-int ldb_bridge_attach_helper(struct drm_bridge *bridge,
-			     enum drm_bridge_attach_flags flags)
-{
-	struct ldb_channel *ldb_ch = bridge->driver_private;
-	struct ldb *ldb = ldb_ch->ldb;
-
-	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
-		DRM_DEV_ERROR(ldb->dev,
-			      "do not support creating a drm_connector\n");
-		return -EINVAL;
-	}
-
-	if (!bridge->encoder) {
-		DRM_DEV_ERROR(ldb->dev, "missing encoder\n");
-		return -ENODEV;
-	}
-
-	return drm_bridge_attach(bridge->encoder,
-				ldb_ch->next_bridge, bridge,
-				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
-}
-
-int ldb_init_helper(struct ldb *ldb)
-{
-	struct device *dev = ldb->dev;
-	struct device_node *np = dev->of_node;
-	struct device_node *child;
-	int ret;
-	u32 i;
-
-	ldb->regmap = syscon_node_to_regmap(np->parent);
-	if (IS_ERR(ldb->regmap)) {
-		ret = PTR_ERR(ldb->regmap);
-		if (ret != -EPROBE_DEFER)
-			DRM_DEV_ERROR(dev, "failed to get regmap: %d\n", ret);
-		return ret;
-	}
-
-	for_each_available_child_of_node(np, child) {
-		struct ldb_channel *ldb_ch;
-
-		ret = of_property_read_u32(child, "reg", &i);
-		if (ret || i > MAX_LDB_CHAN_NUM - 1) {
-			ret = -EINVAL;
-			DRM_DEV_ERROR(dev,
-				      "invalid channel node address: %u\n", i);
-			of_node_put(child);
-			return ret;
-		}
-
-		ldb_ch = ldb->channel[i];
-		ldb_ch->ldb = ldb;
-		ldb_ch->chno = i;
-		ldb_ch->is_available = true;
-		ldb_ch->np = child;
-
-		ldb->available_ch_cnt++;
-	}
-
-	return 0;
-}
-
-int ldb_find_next_bridge_helper(struct ldb *ldb)
-{
-	struct device *dev = ldb->dev;
-	struct ldb_channel *ldb_ch;
-	int ret, i;
-
-	for (i = 0; i < MAX_LDB_CHAN_NUM; i++) {
-		ldb_ch = ldb->channel[i];
-
-		if (!ldb_ch->is_available)
-			continue;
-
-		ldb_ch->next_bridge = devm_drm_of_get_bridge(dev, ldb_ch->np,
-							     1, 0);
-		if (IS_ERR(ldb_ch->next_bridge)) {
-			ret = PTR_ERR(ldb_ch->next_bridge);
-			if (ret != -EPROBE_DEFER)
-				DRM_DEV_ERROR(dev,
-					      "failed to get next bridge: %d\n",
-					      ret);
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
-void ldb_add_bridge_helper(struct ldb *ldb,
-			   const struct drm_bridge_funcs *bridge_funcs)
-{
-	struct ldb_channel *ldb_ch;
-	int i;
-
-	for (i = 0; i < MAX_LDB_CHAN_NUM; i++) {
-		ldb_ch = ldb->channel[i];
-
-		if (!ldb_ch->is_available)
-			continue;
-
-		ldb_ch->bridge.driver_private = ldb_ch;
-		ldb_ch->bridge.funcs = bridge_funcs;
-		ldb_ch->bridge.of_node = ldb_ch->np;
-
-		drm_bridge_add(&ldb_ch->bridge);
-	}
-}
-
-void ldb_remove_bridge_helper(struct ldb *ldb)
-{
-	struct ldb_channel *ldb_ch;
-	int i;
-
-	for (i = 0; i < MAX_LDB_CHAN_NUM; i++) {
-		ldb_ch = ldb->channel[i];
-
-		if (!ldb_ch->is_available)
-			continue;
-
-		drm_bridge_remove(&ldb_ch->bridge);
-	}
-}
diff --git a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.h b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.h
index a0a5cde27fbc..42e9b4aa8399 100644
--- a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.h
+++ b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.h
@@ -65,32 +65,209 @@ struct ldb {
 
 #define bridge_to_ldb_ch(b)	container_of(b, struct ldb_channel, bridge)
 
-bool ldb_channel_is_single_link(struct ldb_channel *ldb_ch);
-bool ldb_channel_is_split_link(struct ldb_channel *ldb_ch);
+static inline bool ldb_channel_is_single_link(struct ldb_channel *ldb_ch)
+{
+	return ldb_ch->link_type == LDB_CH_SINGLE_LINK;
+}
 
-int ldb_bridge_atomic_check_helper(struct drm_bridge *bridge,
-				   struct drm_bridge_state *bridge_state,
-				   struct drm_crtc_state *crtc_state,
-				   struct drm_connector_state *conn_state);
+static inline bool ldb_channel_is_split_link(struct ldb_channel *ldb_ch)
+{
+	return ldb_ch->link_type == LDB_CH_DUAL_LINK_EVEN_ODD_PIXELS ||
+	       ldb_ch->link_type == LDB_CH_DUAL_LINK_ODD_EVEN_PIXELS;
+}
 
-void ldb_bridge_mode_set_helper(struct drm_bridge *bridge,
-				const struct drm_display_mode *mode,
-				const struct drm_display_mode *adjusted_mode);
+static inline int ldb_bridge_atomic_check_helper(struct drm_bridge *bridge,
+						 struct drm_bridge_state *bridge_state,
+						 struct drm_crtc_state *crtc_state,
+						 struct drm_connector_state *conn_state)
+{
+	struct ldb_channel *ldb_ch = bridge->driver_private;
 
-void ldb_bridge_enable_helper(struct drm_bridge *bridge);
+	ldb_ch->in_bus_format = bridge_state->input_bus_cfg.format;
+	ldb_ch->out_bus_format = bridge_state->output_bus_cfg.format;
 
-void ldb_bridge_disable_helper(struct drm_bridge *bridge);
+	return 0;
+}
 
-int ldb_bridge_attach_helper(struct drm_bridge *bridge,
-			     enum drm_bridge_attach_flags flags);
+static inline void ldb_bridge_mode_set_helper(struct drm_bridge *bridge,
+					      const struct drm_display_mode *mode,
+					      const struct drm_display_mode *adjusted_mode)
+{
+	struct ldb_channel *ldb_ch = bridge->driver_private;
+	struct ldb *ldb = ldb_ch->ldb;
+	bool is_split = ldb_channel_is_split_link(ldb_ch);
 
-int ldb_init_helper(struct ldb *ldb);
+	if (is_split)
+		ldb->ldb_ctrl |= LDB_SPLIT_MODE_EN;
 
-int ldb_find_next_bridge_helper(struct ldb *ldb);
+	switch (ldb_ch->out_bus_format) {
+	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
+		break;
+	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
+		if (ldb_ch->chno == 0 || is_split)
+			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24;
+		if (ldb_ch->chno == 1 || is_split)
+			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24;
+		break;
+	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
+		if (ldb_ch->chno == 0 || is_split)
+			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24 |
+					 LDB_BIT_MAP_CH0_JEIDA;
+		if (ldb_ch->chno == 1 || is_split)
+			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24 |
+					 LDB_BIT_MAP_CH1_JEIDA;
+		break;
+	}
+}
 
-void ldb_add_bridge_helper(struct ldb *ldb,
-			   const struct drm_bridge_funcs *bridge_funcs);
+static inline void ldb_bridge_enable_helper(struct drm_bridge *bridge)
+{
+	struct ldb_channel *ldb_ch = bridge->driver_private;
+	struct ldb *ldb = ldb_ch->ldb;
 
-void ldb_remove_bridge_helper(struct ldb *ldb);
+	/*
+	 * Platform specific bridge drivers should set ldb_ctrl properly
+	 * for the enablement, so just write the ctrl_reg here.
+	 */
+	regmap_write(ldb->regmap, ldb->ctrl_reg, ldb->ldb_ctrl);
+}
+
+static inline void ldb_bridge_disable_helper(struct drm_bridge *bridge)
+{
+	struct ldb_channel *ldb_ch = bridge->driver_private;
+	struct ldb *ldb = ldb_ch->ldb;
+	bool is_split = ldb_channel_is_split_link(ldb_ch);
+
+	if (ldb_ch->chno == 0 || is_split)
+		ldb->ldb_ctrl &= ~LDB_CH0_MODE_EN_MASK;
+	if (ldb_ch->chno == 1 || is_split)
+		ldb->ldb_ctrl &= ~LDB_CH1_MODE_EN_MASK;
+
+	regmap_write(ldb->regmap, ldb->ctrl_reg, ldb->ldb_ctrl);
+}
+
+static inline int ldb_bridge_attach_helper(struct drm_bridge *bridge,
+					   enum drm_bridge_attach_flags flags)
+{
+	struct ldb_channel *ldb_ch = bridge->driver_private;
+	struct ldb *ldb = ldb_ch->ldb;
+
+	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
+		DRM_DEV_ERROR(ldb->dev,
+			      "do not support creating a drm_connector\n");
+		return -EINVAL;
+	}
+
+	if (!bridge->encoder) {
+		DRM_DEV_ERROR(ldb->dev, "missing encoder\n");
+		return -ENODEV;
+	}
+
+	return drm_bridge_attach(bridge->encoder,
+				ldb_ch->next_bridge, bridge,
+				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+}
+
+static inline int ldb_init_helper(struct ldb *ldb)
+{
+	struct device *dev = ldb->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *child;
+	int ret;
+	u32 i;
+
+	ldb->regmap = syscon_node_to_regmap(np->parent);
+	if (IS_ERR(ldb->regmap)) {
+		ret = PTR_ERR(ldb->regmap);
+		if (ret != -EPROBE_DEFER)
+			DRM_DEV_ERROR(dev, "failed to get regmap: %d\n", ret);
+		return ret;
+	}
+
+	for_each_available_child_of_node(np, child) {
+		struct ldb_channel *ldb_ch;
+
+		ret = of_property_read_u32(child, "reg", &i);
+		if (ret || i > MAX_LDB_CHAN_NUM - 1) {
+			ret = -EINVAL;
+			DRM_DEV_ERROR(dev,
+				      "invalid channel node address: %u\n", i);
+			of_node_put(child);
+			return ret;
+		}
+
+		ldb_ch = ldb->channel[i];
+		ldb_ch->ldb = ldb;
+		ldb_ch->chno = i;
+		ldb_ch->is_available = true;
+		ldb_ch->np = child;
+
+		ldb->available_ch_cnt++;
+	}
+
+	return 0;
+}
+
+static inline int ldb_find_next_bridge_helper(struct ldb *ldb)
+{
+	struct device *dev = ldb->dev;
+	struct ldb_channel *ldb_ch;
+	int ret, i;
+
+	for (i = 0; i < MAX_LDB_CHAN_NUM; i++) {
+		ldb_ch = ldb->channel[i];
+
+		if (!ldb_ch->is_available)
+			continue;
+
+		ldb_ch->next_bridge = devm_drm_of_get_bridge(dev, ldb_ch->np,
+							     1, 0);
+		if (IS_ERR(ldb_ch->next_bridge)) {
+			ret = PTR_ERR(ldb_ch->next_bridge);
+			if (ret != -EPROBE_DEFER)
+				DRM_DEV_ERROR(dev,
+					      "failed to get next bridge: %d\n",
+					      ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static inline void ldb_add_bridge_helper(struct ldb *ldb,
+					 const struct drm_bridge_funcs *bridge_funcs)
+{
+	struct ldb_channel *ldb_ch;
+	int i;
+
+	for (i = 0; i < MAX_LDB_CHAN_NUM; i++) {
+		ldb_ch = ldb->channel[i];
+
+		if (!ldb_ch->is_available)
+			continue;
+
+		ldb_ch->bridge.driver_private = ldb_ch;
+		ldb_ch->bridge.funcs = bridge_funcs;
+		ldb_ch->bridge.of_node = ldb_ch->np;
+
+		drm_bridge_add(&ldb_ch->bridge);
+	}
+}
+
+static inline void ldb_remove_bridge_helper(struct ldb *ldb)
+{
+	struct ldb_channel *ldb_ch;
+	int i;
+
+	for (i = 0; i < MAX_LDB_CHAN_NUM; i++) {
+		ldb_ch = ldb->channel[i];
+
+		if (!ldb_ch->is_available)
+			continue;
+
+		drm_bridge_remove(&ldb_ch->bridge);
+	}
+}
 
 #endif /* __IMX_LDB_HELPER__ */
-- 
2.39.2

[PATCH 2/2] drm/bridge: imx: turn imx8{qm,qxp}-ldb into single-object modules

[Masahiro Yamada]

With the previous fix, these modules are built from a single C file.

Rename the source files so they match the module names.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 drivers/gpu/drm/bridge/imx/Makefile                           | 4 ----
 drivers/gpu/drm/bridge/imx/{imx8qm-ldb-drv.c => imx8qm-ldb.c} | 0
 .../gpu/drm/bridge/imx/{imx8qxp-ldb-drv.c => imx8qxp-ldb.c}   | 0
 3 files changed, 4 deletions(-)
 rename drivers/gpu/drm/bridge/imx/{imx8qm-ldb-drv.c => imx8qm-ldb.c} (100%)
 rename drivers/gpu/drm/bridge/imx/{imx8qxp-ldb-drv.c => imx8qxp-ldb.c} (100%)

diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile
index 64b93009376a..c102443f7286 100644
--- a/drivers/gpu/drm/bridge/imx/Makefile
+++ b/drivers/gpu/drm/bridge/imx/Makefile
@@ -1,9 +1,5 @@
-imx8qm-ldb-objs := imx8qm-ldb-drv.o
 obj-$(CONFIG_DRM_IMX8QM_LDB) += imx8qm-ldb.o
-
-imx8qxp-ldb-objs := imx8qxp-ldb-drv.o
 obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
-
 obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o
 obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o
 obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-pxl2dpi.o
diff --git a/drivers/gpu/drm/bridge/imx/imx8qm-ldb-drv.c b/drivers/gpu/drm/bridge/imx/imx8qm-ldb.c
similarity index 100%
rename from drivers/gpu/drm/bridge/imx/imx8qm-ldb-drv.c
rename to drivers/gpu/drm/bridge/imx/imx8qm-ldb.c
diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-ldb-drv.c b/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
similarity index 100%
rename from drivers/gpu/drm/bridge/imx/imx8qxp-ldb-drv.c
rename to drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
-- 
2.39.2

Re: [PATCH 1/2] drm/bridge: imx: fix mixed module-builtin object

[Laurent Pinchart]

Hi Yamada-san,

Thank you for the patch.

On Sun, Jun 04, 2023 at 02:07:46AM +0900, Masahiro Yamada wrote:
> With CONFIG_DRM_IMX8QM_LDB=m and CONFIG_DRM_IMX8QXP_LDB=y (or vice
> versa), imx-ldb-helper.o is linked to a module and also to vmlinux
> even though the expected CFLAGS are different between builtins and
> modules.
> 
> This is the same situation as fixed by commit 637a642f5ca5 ("zstd:
> Fixing mixed module-builtin objects").
> 
> Turn helpers in imx-ldb-helper.c into inline functions.

Wouldn't it be better to turn it into a module ? It could then be
built-in for the above configuration, are compiled as a module when all
its users are module as well.

> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  drivers/gpu/drm/bridge/imx/Makefile         |   4 +-
>  drivers/gpu/drm/bridge/imx/imx-ldb-helper.c | 221 --------------------
>  drivers/gpu/drm/bridge/imx/imx-ldb-helper.h | 213 +++++++++++++++++--
>  3 files changed, 197 insertions(+), 241 deletions(-)
>  delete mode 100644 drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> 
> diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile
> index aa90ec8d5433..64b93009376a 100644
> --- a/drivers/gpu/drm/bridge/imx/Makefile
> +++ b/drivers/gpu/drm/bridge/imx/Makefile
> @@ -1,7 +1,7 @@
> -imx8qm-ldb-objs := imx-ldb-helper.o imx8qm-ldb-drv.o
> +imx8qm-ldb-objs := imx8qm-ldb-drv.o
>  obj-$(CONFIG_DRM_IMX8QM_LDB) += imx8qm-ldb.o
>  
> -imx8qxp-ldb-objs := imx-ldb-helper.o imx8qxp-ldb-drv.o
> +imx8qxp-ldb-objs := imx8qxp-ldb-drv.o
>  obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
>  
>  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o
> diff --git a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> deleted file mode 100644
> index 7338b84bc83d..000000000000
> --- a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> +++ /dev/null
> @@ -1,221 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0+
> -/*
> - * Copyright (C) 2012 Sascha Hauer, Pengutronix
> - * Copyright 2019,2020,2022 NXP
> - */
> -
> -#include <linux/media-bus-format.h>
> -#include <linux/mfd/syscon.h>
> -#include <linux/of.h>
> -#include <linux/regmap.h>
> -
> -#include <drm/drm_bridge.h>
> -#include <drm/drm_of.h>
> -#include <drm/drm_print.h>
> -
> -#include "imx-ldb-helper.h"
> -
> -bool ldb_channel_is_single_link(struct ldb_channel *ldb_ch)
> -{
> -	return ldb_ch->link_type == LDB_CH_SINGLE_LINK;
> -}
> -
> -bool ldb_channel_is_split_link(struct ldb_channel *ldb_ch)
> -{
> -	return ldb_ch->link_type == LDB_CH_DUAL_LINK_EVEN_ODD_PIXELS ||
> -	       ldb_ch->link_type == LDB_CH_DUAL_LINK_ODD_EVEN_PIXELS;
> -}
> -
> -int ldb_bridge_atomic_check_helper(struct drm_bridge *bridge,
> -				   struct drm_bridge_state *bridge_state,
> -				   struct drm_crtc_state *crtc_state,
> -				   struct drm_connector_state *conn_state)
> -{
> -	struct ldb_channel *ldb_ch = bridge->driver_private;
> -
> -	ldb_ch->in_bus_format = bridge_state->input_bus_cfg.format;
> -	ldb_ch->out_bus_format = bridge_state->output_bus_cfg.format;
> -
> -	return 0;
> -}
> -
> -void ldb_bridge_mode_set_helper(struct drm_bridge *bridge,
> -				const struct drm_display_mode *mode,
> -				const struct drm_display_mode *adjusted_mode)
> -{
> -	struct ldb_channel *ldb_ch = bridge->driver_private;
> -	struct ldb *ldb = ldb_ch->ldb;
> -	bool is_split = ldb_channel_is_split_link(ldb_ch);
> -
> -	if (is_split)
> -		ldb->ldb_ctrl |= LDB_SPLIT_MODE_EN;
> -
> -	switch (ldb_ch->out_bus_format) {
> -	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> -		break;
> -	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> -		if (ldb_ch->chno == 0 || is_split)
> -			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24;
> -		if (ldb_ch->chno == 1 || is_split)
> -			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24;
> -		break;
> -	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> -		if (ldb_ch->chno == 0 || is_split)
> -			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24 |
> -					 LDB_BIT_MAP_CH0_JEIDA;
> -		if (ldb_ch->chno == 1 || is_split)
> -			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24 |
> -					 LDB_BIT_MAP_CH1_JEIDA;
> -		break;
> -	}
> -}
> -
> -void ldb_bridge_enable_helper(struct drm_bridge *bridge)
> -{
> -	struct ldb_channel *ldb_ch = bridge->driver_private;
> -	struct ldb *ldb = ldb_ch->ldb;
> -
> -	/*
> -	 * Platform specific bridge drivers should set ldb_ctrl properly
> -	 * for the enablement, so just write the ctrl_reg here.
> -	 */
> -	regmap_write(ldb->regmap, ldb->ctrl_reg, ldb->ldb_ctrl);
> -}
> -
> -void ldb_bridge_disable_helper(struct drm_bridge *bridge)
> -{
> -	struct ldb_channel *ldb_ch = bridge->driver_private;
> -	struct ldb *ldb = ldb_ch->ldb;
> -	bool is_split = ldb_channel_is_split_link(ldb_ch);
> -
> -	if (ldb_ch->chno == 0 || is_split)
> -		ldb->ldb_ctrl &= ~LDB_CH0_MODE_EN_MASK;
> -	if (ldb_ch->chno == 1 || is_split)
> -		ldb->ldb_ctrl &= ~LDB_CH1_MODE_EN_MASK;
> -
> -	regmap_write(ldb->regmap, ldb->ctrl_reg, ldb->ldb_ctrl);
> -}
> -
> -int ldb_bridge_attach_helper(struct drm_bridge *bridge,
> -			     enum drm_bridge_attach_flags flags)
> -{
> -	struct ldb_channel *ldb_ch = bridge->driver_private;
> -	struct ldb *ldb = ldb_ch->ldb;
> -
> -	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> -		DRM_DEV_ERROR(ldb->dev,
> -			      "do not support creating a drm_connector\n");
> -		return -EINVAL;
> -	}
> -
> -	if (!bridge->encoder) {
> -		DRM_DEV_ERROR(ldb->dev, "missing encoder\n");
> -		return -ENODEV;
> -	}
> -
> -	return drm_bridge_attach(bridge->encoder,
> -				ldb_ch->next_bridge, bridge,
> -				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> -}
> -
> -int ldb_init_helper(struct ldb *ldb)
> -{
> -	struct device *dev = ldb->dev;
> -	struct device_node *np = dev->of_node;
> -	struct device_node *child;
> -	int ret;
> -	u32 i;
> -
> -	ldb->regmap = syscon_node_to_regmap(np->parent);
> -	if (IS_ERR(ldb->regmap)) {
> -		ret = PTR_ERR(ldb->regmap);
> -		if (ret != -EPROBE_DEFER)
> -			DRM_DEV_ERROR(dev, "failed to get regmap: %d\n", ret);
> -		return ret;
> -	}
> -
> -	for_each_available_child_of_node(np, child) {
> -		struct ldb_channel *ldb_ch;
> -
> -		ret = of_property_read_u32(child, "reg", &i);
> -		if (ret || i > MAX_LDB_CHAN_NUM - 1) {
> -			ret = -EINVAL;
> -			DRM_DEV_ERROR(dev,
> -				      "invalid channel node address: %u\n", i);
> -			of_node_put(child);
> -			return ret;
> -		}
> -
> -		ldb_ch = ldb->channel[i];
> -		ldb_ch->ldb = ldb;
> -		ldb_ch->chno = i;
> -		ldb_ch->is_available = true;
> -		ldb_ch->np = child;
> -
> -		ldb->available_ch_cnt++;
> -	}
> -
> -	return 0;
> -}
> -
> -int ldb_find_next_bridge_helper(struct ldb *ldb)
> -{
> -	struct device *dev = ldb->dev;
> -	struct ldb_channel *ldb_ch;
> -	int ret, i;
> -
> -	for (i = 0; i < MAX_LDB_CHAN_NUM; i++) {
> -		ldb_ch = ldb->channel[i];
> -
> -		if (!ldb_ch->is_available)
> -			continue;
> -
> -		ldb_ch->next_bridge = devm_drm_of_get_bridge(dev, ldb_ch->np,
> -							     1, 0);
> -		if (IS_ERR(ldb_ch->next_bridge)) {
> -			ret = PTR_ERR(ldb_ch->next_bridge);
> -			if (ret != -EPROBE_DEFER)
> -				DRM_DEV_ERROR(dev,
> -					      "failed to get next bridge: %d\n",
> -					      ret);
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
> -void ldb_add_bridge_helper(struct ldb *ldb,
> -			   const struct drm_bridge_funcs *bridge_funcs)
> -{
> -	struct ldb_channel *ldb_ch;
> -	int i;
> -
> -	for (i = 0; i < MAX_LDB_CHAN_NUM; i++) {
> -		ldb_ch = ldb->channel[i];
> -
> -		if (!ldb_ch->is_available)
> -			continue;
> -
> -		ldb_ch->bridge.driver_private = ldb_ch;
> -		ldb_ch->bridge.funcs = bridge_funcs;
> -		ldb_ch->bridge.of_node = ldb_ch->np;
> -
> -		drm_bridge_add(&ldb_ch->bridge);
> -	}
> -}
> -
> -void ldb_remove_bridge_helper(struct ldb *ldb)
> -{
> -	struct ldb_channel *ldb_ch;
> -	int i;
> -
> -	for (i = 0; i < MAX_LDB_CHAN_NUM; i++) {
> -		ldb_ch = ldb->channel[i];
> -
> -		if (!ldb_ch->is_available)
> -			continue;
> -
> -		drm_bridge_remove(&ldb_ch->bridge);
> -	}
> -}
> diff --git a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.h b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.h
> index a0a5cde27fbc..42e9b4aa8399 100644
> --- a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.h
> +++ b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.h
> @@ -65,32 +65,209 @@ struct ldb {
>  
>  #define bridge_to_ldb_ch(b)	container_of(b, struct ldb_channel, bridge)
>  
> -bool ldb_channel_is_single_link(struct ldb_channel *ldb_ch);
> -bool ldb_channel_is_split_link(struct ldb_channel *ldb_ch);
> +static inline bool ldb_channel_is_single_link(struct ldb_channel *ldb_ch)
> +{
> +	return ldb_ch->link_type == LDB_CH_SINGLE_LINK;
> +}
>  
> -int ldb_bridge_atomic_check_helper(struct drm_bridge *bridge,
> -				   struct drm_bridge_state *bridge_state,
> -				   struct drm_crtc_state *crtc_state,
> -				   struct drm_connector_state *conn_state);
> +static inline bool ldb_channel_is_split_link(struct ldb_channel *ldb_ch)
> +{
> +	return ldb_ch->link_type == LDB_CH_DUAL_LINK_EVEN_ODD_PIXELS ||
> +	       ldb_ch->link_type == LDB_CH_DUAL_LINK_ODD_EVEN_PIXELS;
> +}
>  
> -void ldb_bridge_mode_set_helper(struct drm_bridge *bridge,
> -				const struct drm_display_mode *mode,
> -				const struct drm_display_mode *adjusted_mode);
> +static inline int ldb_bridge_atomic_check_helper(struct drm_bridge *bridge,
> +						 struct drm_bridge_state *bridge_state,
> +						 struct drm_crtc_state *crtc_state,
> +						 struct drm_connector_state *conn_state)
> +{
> +	struct ldb_channel *ldb_ch = bridge->driver_private;
>  
> -void ldb_bridge_enable_helper(struct drm_bridge *bridge);
> +	ldb_ch->in_bus_format = bridge_state->input_bus_cfg.format;
> +	ldb_ch->out_bus_format = bridge_state->output_bus_cfg.format;
>  
> -void ldb_bridge_disable_helper(struct drm_bridge *bridge);
> +	return 0;
> +}
>  
> -int ldb_bridge_attach_helper(struct drm_bridge *bridge,
> -			     enum drm_bridge_attach_flags flags);
> +static inline void ldb_bridge_mode_set_helper(struct drm_bridge *bridge,
> +					      const struct drm_display_mode *mode,
> +					      const struct drm_display_mode *adjusted_mode)
> +{
> +	struct ldb_channel *ldb_ch = bridge->driver_private;
> +	struct ldb *ldb = ldb_ch->ldb;
> +	bool is_split = ldb_channel_is_split_link(ldb_ch);
>  
> -int ldb_init_helper(struct ldb *ldb);
> +	if (is_split)
> +		ldb->ldb_ctrl |= LDB_SPLIT_MODE_EN;
>  
> -int ldb_find_next_bridge_helper(struct ldb *ldb);
> +	switch (ldb_ch->out_bus_format) {
> +	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> +		break;
> +	case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> +		if (ldb_ch->chno == 0 || is_split)
> +			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24;
> +		if (ldb_ch->chno == 1 || is_split)
> +			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24;
> +		break;
> +	case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> +		if (ldb_ch->chno == 0 || is_split)
> +			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24 |
> +					 LDB_BIT_MAP_CH0_JEIDA;
> +		if (ldb_ch->chno == 1 || is_split)
> +			ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24 |
> +					 LDB_BIT_MAP_CH1_JEIDA;
> +		break;
> +	}
> +}
>  
> -void ldb_add_bridge_helper(struct ldb *ldb,
> -			   const struct drm_bridge_funcs *bridge_funcs);
> +static inline void ldb_bridge_enable_helper(struct drm_bridge *bridge)
> +{
> +	struct ldb_channel *ldb_ch = bridge->driver_private;
> +	struct ldb *ldb = ldb_ch->ldb;
>  
> -void ldb_remove_bridge_helper(struct ldb *ldb);
> +	/*
> +	 * Platform specific bridge drivers should set ldb_ctrl properly
> +	 * for the enablement, so just write the ctrl_reg here.
> +	 */
> +	regmap_write(ldb->regmap, ldb->ctrl_reg, ldb->ldb_ctrl);
> +}
> +
> +static inline void ldb_bridge_disable_helper(struct drm_bridge *bridge)
> +{
> +	struct ldb_channel *ldb_ch = bridge->driver_private;
> +	struct ldb *ldb = ldb_ch->ldb;
> +	bool is_split = ldb_channel_is_split_link(ldb_ch);
> +
> +	if (ldb_ch->chno == 0 || is_split)
> +		ldb->ldb_ctrl &= ~LDB_CH0_MODE_EN_MASK;
> +	if (ldb_ch->chno == 1 || is_split)
> +		ldb->ldb_ctrl &= ~LDB_CH1_MODE_EN_MASK;
> +
> +	regmap_write(ldb->regmap, ldb->ctrl_reg, ldb->ldb_ctrl);
> +}
> +
> +static inline int ldb_bridge_attach_helper(struct drm_bridge *bridge,
> +					   enum drm_bridge_attach_flags flags)
> +{
> +	struct ldb_channel *ldb_ch = bridge->driver_private;
> +	struct ldb *ldb = ldb_ch->ldb;
> +
> +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> +		DRM_DEV_ERROR(ldb->dev,
> +			      "do not support creating a drm_connector\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!bridge->encoder) {
> +		DRM_DEV_ERROR(ldb->dev, "missing encoder\n");
> +		return -ENODEV;
> +	}
> +
> +	return drm_bridge_attach(bridge->encoder,
> +				ldb_ch->next_bridge, bridge,
> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +}
> +
> +static inline int ldb_init_helper(struct ldb *ldb)
> +{
> +	struct device *dev = ldb->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *child;
> +	int ret;
> +	u32 i;
> +
> +	ldb->regmap = syscon_node_to_regmap(np->parent);
> +	if (IS_ERR(ldb->regmap)) {
> +		ret = PTR_ERR(ldb->regmap);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev, "failed to get regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	for_each_available_child_of_node(np, child) {
> +		struct ldb_channel *ldb_ch;
> +
> +		ret = of_property_read_u32(child, "reg", &i);
> +		if (ret || i > MAX_LDB_CHAN_NUM - 1) {
> +			ret = -EINVAL;
> +			DRM_DEV_ERROR(dev,
> +				      "invalid channel node address: %u\n", i);
> +			of_node_put(child);
> +			return ret;
> +		}
> +
> +		ldb_ch = ldb->channel[i];
> +		ldb_ch->ldb = ldb;
> +		ldb_ch->chno = i;
> +		ldb_ch->is_available = true;
> +		ldb_ch->np = child;
> +
> +		ldb->available_ch_cnt++;
> +	}
> +
> +	return 0;
> +}
> +
> +static inline int ldb_find_next_bridge_helper(struct ldb *ldb)
> +{
> +	struct device *dev = ldb->dev;
> +	struct ldb_channel *ldb_ch;
> +	int ret, i;
> +
> +	for (i = 0; i < MAX_LDB_CHAN_NUM; i++) {
> +		ldb_ch = ldb->channel[i];
> +
> +		if (!ldb_ch->is_available)
> +			continue;
> +
> +		ldb_ch->next_bridge = devm_drm_of_get_bridge(dev, ldb_ch->np,
> +							     1, 0);
> +		if (IS_ERR(ldb_ch->next_bridge)) {
> +			ret = PTR_ERR(ldb_ch->next_bridge);
> +			if (ret != -EPROBE_DEFER)
> +				DRM_DEV_ERROR(dev,
> +					      "failed to get next bridge: %d\n",
> +					      ret);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static inline void ldb_add_bridge_helper(struct ldb *ldb,
> +					 const struct drm_bridge_funcs *bridge_funcs)
> +{
> +	struct ldb_channel *ldb_ch;
> +	int i;
> +
> +	for (i = 0; i < MAX_LDB_CHAN_NUM; i++) {
> +		ldb_ch = ldb->channel[i];
> +
> +		if (!ldb_ch->is_available)
> +			continue;
> +
> +		ldb_ch->bridge.driver_private = ldb_ch;
> +		ldb_ch->bridge.funcs = bridge_funcs;
> +		ldb_ch->bridge.of_node = ldb_ch->np;
> +
> +		drm_bridge_add(&ldb_ch->bridge);
> +	}
> +}
> +
> +static inline void ldb_remove_bridge_helper(struct ldb *ldb)
> +{
> +	struct ldb_channel *ldb_ch;
> +	int i;
> +
> +	for (i = 0; i < MAX_LDB_CHAN_NUM; i++) {
> +		ldb_ch = ldb->channel[i];
> +
> +		if (!ldb_ch->is_available)
> +			continue;
> +
> +		drm_bridge_remove(&ldb_ch->bridge);
> +	}
> +}
>  
>  #endif /* __IMX_LDB_HELPER__ */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] drm/bridge: imx: fix mixed module-builtin object

[Masahiro Yamada]

On Sun, Jun 4, 2023 at 1:52 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Yamada-san,
>
> Thank you for the patch.
>
> On Sun, Jun 04, 2023 at 02:07:46AM +0900, Masahiro Yamada wrote:
> > With CONFIG_DRM_IMX8QM_LDB=m and CONFIG_DRM_IMX8QXP_LDB=y (or vice
> > versa), imx-ldb-helper.o is linked to a module and also to vmlinux
> > even though the expected CFLAGS are different between builtins and
> > modules.
> >
> > This is the same situation as fixed by commit 637a642f5ca5 ("zstd:
> > Fixing mixed module-builtin objects").
> >
> > Turn helpers in imx-ldb-helper.c into inline functions.
>
> Wouldn't it be better to turn it into a module ? It could then be
> built-in for the above configuration, are compiled as a module when all
> its users are module as well.


Yes, two ways to fix it.
inline line functions vs a separate module

I do not have a strong opinion.


I sent v2.
https://lore.kernel.org/lkml/20230604075713.1027261-1-masahiroy@kernel.org/T/#t

Please pick a preferred one.








--
Best Regards

Masahiro Yamada