[PATCH 0/8] Allow link the it66121 display bridge driver as a lib

[PATCH 0/8] Allow link the it66121 display bridge driver as a lib

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

The goal is to make this driver usuable on non-DT based systems, while
without introduce duplication and preserve code sharing. The first seven
patch just doing trivial cleanup, the last patch try to solve the mentioned
problem by allowing static linking. Tested on a ACPI-based platform:

$ dmseg | grep drm

 ACPI: bus type drm_connector registered
 loongson 0000:00:06.1: [drm] dc: 264MHz, gmc: 529MHz, gpu: 529MHz
 loongson 0000:00:06.1: [drm] Dedicated vram start: 0xe0030000000, size: 64MiB
 loongson 0000:00:06.1: [drm] Loongson VBIOS version: 0.3
 loongson 0000:00:06.1: [drm] Loongson VBIOS: has 8 DCBs
 loongson 0000:00:06.1: [drm] VRAM: 4096 pages ready
 loongson 0000:00:06.1: [drm] GTT: 32768 pages ready
 loongson 0000:00:06.1: [drm] lsdc-i2c0(sda pin mask=1, scl pin mask=2) created
 loongson 0000:00:06.1: [drm] lsdc-i2c1(sda pin mask=4, scl pin mask=8) created
 loongson 0000:00:06.1: [drm] DisplayPipe-0 has DVO-0 connector
 i2c 1-004c: IT66121 probed, chip id: 0x4954:0x612, revision: 1
 loongson 0000:00:06.1: [drm] DisplayPipe-1 has IT66121@4c attached
 loongson 0000:00:06.1: [drm] Total 2 outputs
 loongson 0000:00:06.1: [drm] registered irq: 40
 [drm] Initialized loongson 1.0.0 20220701 for 0000:00:06.1 on minor 0
 loongson 0000:00:06.1: [drm] fb0: loongsondrmfb frame buffer device

Sui Jingfeng (8):
  drm/bridge: it66121: Use dev replace ctx->dev in the it66121_probe()
  drm/bridge: it66121: Add bridge_to_it66121() helper and use it
  drm/bridge: it66121: Add a helper function to read bus width
  drm/bridge: it66121: Add a helper function to get the next bridge
  drm/bridge: it66121: Add a helper function to read chip id
  drm/bridge: it66121: Add a helper to initialize the DRM bridge
    structure
  drm/bridge: it66121: Add another implementation for getting match data
  drm/bridge: it66121: Allow link this driver as a lib

 drivers/gpu/drm/bridge/ite-it66121.c | 340 +++++++++++++++++++--------
 include/drm/bridge/ite-it66121.h     |  17 ++
 2 files changed, 258 insertions(+), 99 deletions(-)
 create mode 100644 include/drm/bridge/ite-it66121.h

-- 
2.34.1

[PATCH 1/8] drm/bridge: it66121: Use dev replace ctx->dev in the it66121_probe()

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

As the value of 'ctx->dev' has already been cached to the local variable
'dev', so keep the usage consistent. Just trivial cleanup, no functional
change.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 1cf3fb1f13dc..6e5c10cfa0bc 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -1536,20 +1536,20 @@ static int it66121_probe(struct i2c_client *client)
 
 	ep = of_graph_get_remote_node(dev->of_node, 1, -1);
 	if (!ep) {
-		dev_err(ctx->dev, "The endpoint is unconnected\n");
+		dev_err(dev, "The endpoint is unconnected\n");
 		return -EINVAL;
 	}
 
 	if (!of_device_is_available(ep)) {
 		of_node_put(ep);
-		dev_err(ctx->dev, "The remote device is disabled\n");
+		dev_err(dev, "The remote device is disabled\n");
 		return -ENODEV;
 	}
 
 	ctx->next_bridge = of_drm_find_bridge(ep);
 	of_node_put(ep);
 	if (!ctx->next_bridge) {
-		dev_dbg(ctx->dev, "Next bridge not found, deferring probe\n");
+		dev_dbg(dev, "Next bridge not found, deferring probe\n");
 		return -EPROBE_DEFER;
 	}
 
@@ -1599,7 +1599,7 @@ static int it66121_probe(struct i2c_client *client)
 
 	drm_bridge_add(&ctx->bridge);
 
-	dev_info(ctx->dev, "IT66121 revision %d probed\n", revision_id);
+	dev_info(dev, "IT66121 revision %d probed\n", revision_id);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH 1/8] drm/bridge: it66121: Use dev replace ctx->dev in the it66121_probe()

[Dmitry Baryshkov]

On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> As the value of 'ctx->dev' has already been cached to the local variable
> 'dev', so keep the usage consistent. Just trivial cleanup, no functional
> change.
>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

[PATCH 2/8] drm/bridge: it66121: Add bridge_to_it66121() helper and use it

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Instead of using the container_of() directly, which avoid the code lines
too long in horizontial. Just trivial cleanup, no functional change.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 6e5c10cfa0bc..83dbdbfc9ed8 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -315,6 +315,11 @@ struct it66121_ctx {
 	const struct it66121_chip_info *info;
 };
 
+static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct it66121_ctx, bridge);
+}
+
 static const struct regmap_range_cfg it66121_regmap_banks[] = {
 	{
 		.name = "it66121",
@@ -588,7 +593,7 @@ static bool it66121_is_hpd_detect(struct it66121_ctx *ctx)
 static int it66121_bridge_attach(struct drm_bridge *bridge,
 				 enum drm_bridge_attach_flags flags)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121(bridge);
 	int ret;
 
 	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
@@ -699,7 +704,7 @@ static u32 *it66121_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
 						     u32 output_fmt,
 						     unsigned int *num_input_fmts)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121(bridge);
 	u32 *input_fmts;
 
 	*num_input_fmts = 0;
@@ -723,7 +728,7 @@ static u32 *it66121_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
 static void it66121_bridge_enable(struct drm_bridge *bridge,
 				  struct drm_bridge_state *bridge_state)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121(bridge);
 	struct drm_atomic_state *state = bridge_state->base.state;
 
 	ctx->connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
@@ -734,7 +739,7 @@ static void it66121_bridge_enable(struct drm_bridge *bridge,
 static void it66121_bridge_disable(struct drm_bridge *bridge,
 				   struct drm_bridge_state *bridge_state)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121(bridge);
 
 	it66121_set_mute(ctx, true);
 
@@ -746,7 +751,7 @@ static int it66121_bridge_check(struct drm_bridge *bridge,
 				struct drm_crtc_state *crtc_state,
 				struct drm_connector_state *conn_state)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121(bridge);
 
 	if (ctx->info->id == ID_IT6610) {
 		/* The IT6610 only supports these settings */
@@ -765,7 +770,7 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
 			     const struct drm_display_mode *adjusted_mode)
 {
 	u8 buf[HDMI_INFOFRAME_SIZE(AVI)];
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121(bridge);
 	int ret;
 
 	mutex_lock(&ctx->lock);
@@ -831,7 +836,7 @@ static enum drm_mode_status it66121_bridge_mode_valid(struct drm_bridge *bridge,
 						      const struct drm_display_info *info,
 						      const struct drm_display_mode *mode)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121(bridge);
 	unsigned long max_clock;
 
 	max_clock = (ctx->bus_width == 12) ? 74250 : 148500;
@@ -847,7 +852,7 @@ static enum drm_mode_status it66121_bridge_mode_valid(struct drm_bridge *bridge,
 
 static enum drm_connector_status it66121_bridge_detect(struct drm_bridge *bridge)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121(bridge);
 
 	return it66121_is_hpd_detect(ctx) ? connector_status_connected
 					  : connector_status_disconnected;
@@ -855,7 +860,7 @@ static enum drm_connector_status it66121_bridge_detect(struct drm_bridge *bridge
 
 static void it66121_bridge_hpd_enable(struct drm_bridge *bridge)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121(bridge);
 	int ret;
 
 	ret = regmap_write_bits(ctx->regmap, IT66121_INT_MASK1_REG, IT66121_INT_MASK1_HPD, 0);
@@ -865,7 +870,7 @@ static void it66121_bridge_hpd_enable(struct drm_bridge *bridge)
 
 static void it66121_bridge_hpd_disable(struct drm_bridge *bridge)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121(bridge);
 	int ret;
 
 	ret = regmap_write_bits(ctx->regmap, IT66121_INT_MASK1_REG,
@@ -877,7 +882,7 @@ static void it66121_bridge_hpd_disable(struct drm_bridge *bridge)
 static struct edid *it66121_bridge_get_edid(struct drm_bridge *bridge,
 					    struct drm_connector *connector)
 {
-	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
+	struct it66121_ctx *ctx = bridge_to_it66121(bridge);
 	struct edid *edid;
 	int ret;
 
-- 
2.34.1

Re: [PATCH 2/8] drm/bridge: it66121: Add bridge_to_it66121() helper and use it

[Dmitry Baryshkov]

On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> Instead of using the container_of() directly, which avoid the code lines
> too long in horizontial. Just trivial cleanup, no functional change.
>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)

-- 
With best wishes
Dmitry

[PATCH 3/8] drm/bridge: it66121: Add a helper function to read bus width

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Group those relavent code lines (which with common purpose) into one helper
function, suppress the dependency on DT to function level. Just trivial
cleanup, no functional change.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 32 ++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 83dbdbfc9ed8..0f78737adc83 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -320,6 +320,26 @@ static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge)
 	return container_of(bridge, struct it66121_ctx, bridge);
 }
 
+static int it66121_of_read_bus_width(struct device *dev, u32 *bus_width)
+{
+	struct device_node *np;
+	u32 bw;
+
+	np = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
+	if (!np)
+		return -EINVAL;
+
+	of_property_read_u32(np, "bus-width", &bw);
+	of_node_put(np);
+
+	if (bw != 12 && bw != 24)
+		return -EINVAL;
+
+	*bus_width = bw;
+
+	return 0;
+}
+
 static const struct regmap_range_cfg it66121_regmap_banks[] = {
 	{
 		.name = "it66121",
@@ -1525,19 +1545,13 @@ static int it66121_probe(struct i2c_client *client)
 	if (!ctx)
 		return -ENOMEM;
 
-	ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
-	if (!ep)
-		return -EINVAL;
-
 	ctx->dev = dev;
 	ctx->client = client;
 	ctx->info = i2c_get_match_data(client);
 
-	of_property_read_u32(ep, "bus-width", &ctx->bus_width);
-	of_node_put(ep);
-
-	if (ctx->bus_width != 12 && ctx->bus_width != 24)
-		return -EINVAL;
+	ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
+	if (ret)
+		return ret;
 
 	ep = of_graph_get_remote_node(dev->of_node, 1, -1);
 	if (!ep) {
-- 
2.34.1

Re: [PATCH 3/8] drm/bridge: it66121: Add a helper function to read bus width

[Dmitry Baryshkov]

On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> Group those relavent code lines (which with common purpose) into one helper
> function, suppress the dependency on DT to function level. Just trivial
> cleanup, no functional change.
>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 32 ++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 83dbdbfc9ed8..0f78737adc83 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -320,6 +320,26 @@ static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge)
>         return container_of(bridge, struct it66121_ctx, bridge);
>  }
>
> +static int it66121_of_read_bus_width(struct device *dev, u32 *bus_width)

Using a pointer to return int value doesn't look right. Just return a
signed int here and if it is not an error, assign it to ctx->bus_width

> +{
> +       struct device_node *np;
> +       u32 bw;
> +
> +       np = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> +       if (!np)
> +               return -EINVAL;
> +
> +       of_property_read_u32(np, "bus-width", &bw);
> +       of_node_put(np);
> +
> +       if (bw != 12 && bw != 24)
> +               return -EINVAL;
> +
> +       *bus_width = bw;
> +
> +       return 0;
> +}
> +
>  static const struct regmap_range_cfg it66121_regmap_banks[] = {
>         {
>                 .name = "it66121",
> @@ -1525,19 +1545,13 @@ static int it66121_probe(struct i2c_client *client)
>         if (!ctx)
>                 return -ENOMEM;
>
> -       ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> -       if (!ep)
> -               return -EINVAL;
> -
>         ctx->dev = dev;
>         ctx->client = client;
>         ctx->info = i2c_get_match_data(client);
>
> -       of_property_read_u32(ep, "bus-width", &ctx->bus_width);
> -       of_node_put(ep);
> -
> -       if (ctx->bus_width != 12 && ctx->bus_width != 24)
> -               return -EINVAL;
> +       ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
> +       if (ret)
> +               return ret;
>
>         ep = of_graph_get_remote_node(dev->of_node, 1, -1);
>         if (!ep) {
> --
> 2.34.1
>


-- 
With best wishes
Dmitry

[PATCH 4/8] drm/bridge: it66121: Add a helper function to get the next bridge

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Group the code lines(which with the same functional) into one dedicated
function, which reduce the weight of it66121_probe() function. Just trivial
cleanuo, no functional change.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 53 ++++++++++++++++++----------
 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 0f78737adc83..7e473beefc79 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -340,6 +340,37 @@ static int it66121_of_read_bus_width(struct device *dev, u32 *bus_width)
 	return 0;
 }
 
+static int it66121_of_get_next_bridge(struct device *dev,
+				      struct drm_bridge **next_bridge)
+{
+	struct device_node *np;
+	struct drm_bridge *bridge;
+
+	np = of_graph_get_remote_node(dev->of_node, 1, -1);
+	if (!np) {
+		dev_err(dev, "The endpoint is unconnected\n");
+		return -EINVAL;
+	}
+
+	if (!of_device_is_available(np)) {
+		of_node_put(np);
+		dev_err(dev, "The remote device is disabled\n");
+		return -ENODEV;
+	}
+
+	bridge = of_drm_find_bridge(np);
+	of_node_put(np);
+
+	if (!bridge) {
+		dev_dbg(dev, "Next bridge not found, deferring probe\n");
+		return -EPROBE_DEFER;
+	}
+
+	*next_bridge = bridge;
+
+	return 0;
+}
+
 static const struct regmap_range_cfg it66121_regmap_banks[] = {
 	{
 		.name = "it66121",
@@ -1531,7 +1562,6 @@ static const char * const it66121_supplies[] = {
 static int it66121_probe(struct i2c_client *client)
 {
 	u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
-	struct device_node *ep;
 	int ret;
 	struct it66121_ctx *ctx;
 	struct device *dev = &client->dev;
@@ -1553,24 +1583,9 @@ static int it66121_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	ep = of_graph_get_remote_node(dev->of_node, 1, -1);
-	if (!ep) {
-		dev_err(dev, "The endpoint is unconnected\n");
-		return -EINVAL;
-	}
-
-	if (!of_device_is_available(ep)) {
-		of_node_put(ep);
-		dev_err(dev, "The remote device is disabled\n");
-		return -ENODEV;
-	}
-
-	ctx->next_bridge = of_drm_find_bridge(ep);
-	of_node_put(ep);
-	if (!ctx->next_bridge) {
-		dev_dbg(dev, "Next bridge not found, deferring probe\n");
-		return -EPROBE_DEFER;
-	}
+	ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
+	if (ret)
+		return ret;
 
 	i2c_set_clientdata(client, ctx);
 	mutex_init(&ctx->lock);
-- 
2.34.1

Re: [PATCH 4/8] drm/bridge: it66121: Add a helper function to get the next bridge

[Dmitry Baryshkov]

On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> Group the code lines(which with the same functional) into one dedicated
> function, which reduce the weight of it66121_probe() function. Just trivial
> cleanuo, no functional change.
>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 53 ++++++++++++++++++----------
>  1 file changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 0f78737adc83..7e473beefc79 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -340,6 +340,37 @@ static int it66121_of_read_bus_width(struct device *dev, u32 *bus_width)
>         return 0;
>  }
>
> +static int it66121_of_get_next_bridge(struct device *dev,
> +                                     struct drm_bridge **next_bridge)

it already exists and it is called drm_of_find_panel_or_bridge(),
could you please use it instead?

> +{
> +       struct device_node *np;
> +       struct drm_bridge *bridge;
> +
> +       np = of_graph_get_remote_node(dev->of_node, 1, -1);
> +       if (!np) {
> +               dev_err(dev, "The endpoint is unconnected\n");
> +               return -EINVAL;
> +       }
> +
> +       if (!of_device_is_available(np)) {
> +               of_node_put(np);
> +               dev_err(dev, "The remote device is disabled\n");
> +               return -ENODEV;
> +       }
> +
> +       bridge = of_drm_find_bridge(np);
> +       of_node_put(np);
> +
> +       if (!bridge) {
> +               dev_dbg(dev, "Next bridge not found, deferring probe\n");
> +               return -EPROBE_DEFER;
> +       }
> +
> +       *next_bridge = bridge;
> +
> +       return 0;
> +}
> +
>  static const struct regmap_range_cfg it66121_regmap_banks[] = {
>         {
>                 .name = "it66121",
> @@ -1531,7 +1562,6 @@ static const char * const it66121_supplies[] = {
>  static int it66121_probe(struct i2c_client *client)
>  {
>         u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
> -       struct device_node *ep;
>         int ret;
>         struct it66121_ctx *ctx;
>         struct device *dev = &client->dev;
> @@ -1553,24 +1583,9 @@ static int it66121_probe(struct i2c_client *client)
>         if (ret)
>                 return ret;
>
> -       ep = of_graph_get_remote_node(dev->of_node, 1, -1);
> -       if (!ep) {
> -               dev_err(dev, "The endpoint is unconnected\n");
> -               return -EINVAL;
> -       }
> -
> -       if (!of_device_is_available(ep)) {
> -               of_node_put(ep);
> -               dev_err(dev, "The remote device is disabled\n");
> -               return -ENODEV;
> -       }
> -
> -       ctx->next_bridge = of_drm_find_bridge(ep);
> -       of_node_put(ep);
> -       if (!ctx->next_bridge) {
> -               dev_dbg(dev, "Next bridge not found, deferring probe\n");
> -               return -EPROBE_DEFER;
> -       }
> +       ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
> +       if (ret)
> +               return ret;
>
>         i2c_set_clientdata(client, ctx);
>         mutex_init(&ctx->lock);
> --
> 2.34.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 4/8] drm/bridge: it66121: Add a helper function to get the next bridge

[Sui Jingfeng]

Hi,


On 2023/11/15 00:05, Dmitry Baryshkov wrote:
> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> Group the code lines(which with the same functional) into one dedicated
>> function, which reduce the weight of it66121_probe() function. Just trivial
>> cleanuo, no functional change.
>>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/bridge/ite-it66121.c | 53 ++++++++++++++++++----------
>>   1 file changed, 34 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
>> index 0f78737adc83..7e473beefc79 100644
>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
>> @@ -340,6 +340,37 @@ static int it66121_of_read_bus_width(struct device *dev, u32 *bus_width)
>>          return 0;
>>   }
>>
>> +static int it66121_of_get_next_bridge(struct device *dev,
>> +                                     struct drm_bridge **next_bridge)
> it already exists and it is called drm_of_find_panel_or_bridge(),
> could you please use it instead?

That function is too fat and tangled, and should be untangled.
it66121 can not connect with a panel, this is a prior knowledge
and is known at compile time. So this prior knowledge shouldn't
be dropped.

>> +{
>> +       struct device_node *np;
>> +       struct drm_bridge *bridge;
>> +
>> +       np = of_graph_get_remote_node(dev->of_node, 1, -1);
>> +       if (!np) {
>> +               dev_err(dev, "The endpoint is unconnected\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (!of_device_is_available(np)) {
>> +               of_node_put(np);
>> +               dev_err(dev, "The remote device is disabled\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       bridge = of_drm_find_bridge(np);
>> +       of_node_put(np);
>> +
>> +       if (!bridge) {
>> +               dev_dbg(dev, "Next bridge not found, deferring probe\n");
>> +               return -EPROBE_DEFER;
>> +       }
>> +
>> +       *next_bridge = bridge;
>> +
>> +       return 0;
>> +}
>> +
>>   static const struct regmap_range_cfg it66121_regmap_banks[] = {
>>          {
>>                  .name = "it66121",
>> @@ -1531,7 +1562,6 @@ static const char * const it66121_supplies[] = {
>>   static int it66121_probe(struct i2c_client *client)
>>   {
>>          u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
>> -       struct device_node *ep;
>>          int ret;
>>          struct it66121_ctx *ctx;
>>          struct device *dev = &client->dev;
>> @@ -1553,24 +1583,9 @@ static int it66121_probe(struct i2c_client *client)
>>          if (ret)
>>                  return ret;
>>
>> -       ep = of_graph_get_remote_node(dev->of_node, 1, -1);
>> -       if (!ep) {
>> -               dev_err(dev, "The endpoint is unconnected\n");
>> -               return -EINVAL;
>> -       }
>> -
>> -       if (!of_device_is_available(ep)) {
>> -               of_node_put(ep);
>> -               dev_err(dev, "The remote device is disabled\n");
>> -               return -ENODEV;
>> -       }
>> -
>> -       ctx->next_bridge = of_drm_find_bridge(ep);
>> -       of_node_put(ep);
>> -       if (!ctx->next_bridge) {
>> -               dev_dbg(dev, "Next bridge not found, deferring probe\n");
>> -               return -EPROBE_DEFER;
>> -       }
>> +       ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
>> +       if (ret)
>> +               return ret;
>>
>>          i2c_set_clientdata(client, ctx);
>>          mutex_init(&ctx->lock);
>> --
>> 2.34.1
>>
>

Re: [PATCH 4/8] drm/bridge: it66121: Add a helper function to get the next bridge

[Dmitry Baryshkov]

On Thu, 23 Nov 2023 at 07:25, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Hi,
>
>
> On 2023/11/15 00:05, Dmitry Baryshkov wrote:
> > On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> >> From: Sui Jingfeng <suijingfeng@loongson.cn>
> >>
> >> Group the code lines(which with the same functional) into one dedicated
> >> function, which reduce the weight of it66121_probe() function. Just trivial
> >> cleanuo, no functional change.
> >>
> >> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> >> ---
> >>   drivers/gpu/drm/bridge/ite-it66121.c | 53 ++++++++++++++++++----------
> >>   1 file changed, 34 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> >> index 0f78737adc83..7e473beefc79 100644
> >> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> >> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> >> @@ -340,6 +340,37 @@ static int it66121_of_read_bus_width(struct device *dev, u32 *bus_width)
> >>          return 0;
> >>   }
> >>
> >> +static int it66121_of_get_next_bridge(struct device *dev,
> >> +                                     struct drm_bridge **next_bridge)
> > it already exists and it is called drm_of_find_panel_or_bridge(),
> > could you please use it instead?
>
> That function is too fat and tangled, and should be untangled.
> it66121 can not connect with a panel, this is a prior knowledge
> and is known at compile time. So this prior knowledge shouldn't
> be dropped.

This prior knowledge is kept by passing NULL as a panel. We already
have a helper. It covers your use case. There is no need to write your
own boilerplate code for it.

>
> >> +{
> >> +       struct device_node *np;
> >> +       struct drm_bridge *bridge;
> >> +
> >> +       np = of_graph_get_remote_node(dev->of_node, 1, -1);
> >> +       if (!np) {
> >> +               dev_err(dev, "The endpoint is unconnected\n");
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       if (!of_device_is_available(np)) {
> >> +               of_node_put(np);
> >> +               dev_err(dev, "The remote device is disabled\n");
> >> +               return -ENODEV;
> >> +       }
> >> +
> >> +       bridge = of_drm_find_bridge(np);
> >> +       of_node_put(np);
> >> +
> >> +       if (!bridge) {
> >> +               dev_dbg(dev, "Next bridge not found, deferring probe\n");
> >> +               return -EPROBE_DEFER;
> >> +       }
> >> +
> >> +       *next_bridge = bridge;
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>   static const struct regmap_range_cfg it66121_regmap_banks[] = {
> >>          {
> >>                  .name = "it66121",
> >> @@ -1531,7 +1562,6 @@ static const char * const it66121_supplies[] = {
> >>   static int it66121_probe(struct i2c_client *client)
> >>   {
> >>          u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
> >> -       struct device_node *ep;
> >>          int ret;
> >>          struct it66121_ctx *ctx;
> >>          struct device *dev = &client->dev;
> >> @@ -1553,24 +1583,9 @@ static int it66121_probe(struct i2c_client *client)
> >>          if (ret)
> >>                  return ret;
> >>
> >> -       ep = of_graph_get_remote_node(dev->of_node, 1, -1);
> >> -       if (!ep) {
> >> -               dev_err(dev, "The endpoint is unconnected\n");
> >> -               return -EINVAL;
> >> -       }
> >> -
> >> -       if (!of_device_is_available(ep)) {
> >> -               of_node_put(ep);
> >> -               dev_err(dev, "The remote device is disabled\n");
> >> -               return -ENODEV;
> >> -       }
> >> -
> >> -       ctx->next_bridge = of_drm_find_bridge(ep);
> >> -       of_node_put(ep);
> >> -       if (!ctx->next_bridge) {
> >> -               dev_dbg(dev, "Next bridge not found, deferring probe\n");
> >> -               return -EPROBE_DEFER;
> >> -       }
> >> +       ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
> >> +       if (ret)
> >> +               return ret;
> >>
> >>          i2c_set_clientdata(client, ctx);
> >>          mutex_init(&ctx->lock);
> >> --
> >> 2.34.1
> >>
> >



-- 
With best wishes
Dmitry

[PATCH 5/8] drm/bridge: it66121: Add a helper function to read chip id

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Read the required chip id data back by calling regmap_bulk_read() once,
reduce the number of local variables needed in it66121_probe() function.
And store its values into struct it66121_ctx, as it will be used latter.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 7e473beefc79..f36d05331f25 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -313,6 +313,9 @@ struct it66121_ctx {
 		bool auto_cts;
 	} audio;
 	const struct it66121_chip_info *info;
+	u16 vender_id;
+	u16 device_id;
+	u8 revision;
 };
 
 static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge)
@@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx)
 	gpiod_set_value(ctx->gpio_reset, 0);
 }
 
+static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose)
+{
+	u8 id[4];
+	int ret;
+
+	ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4);
+	if (ret < 0) {
+		dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret);
+		return ret;
+	}
+
+	ctx->vender_id = (u16)id[1] << 8 | id[0];
+	ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]);
+	/* Revision is shared with DEVICE_ID1 */
+	ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]);
+
+	if (verbose) {
+		dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n",
+			 ctx->vender_id, ctx->device_id, ctx->revision);
+	}
+
+	return 0;
+}
+
 static inline int it66121_preamble_ddc(struct it66121_ctx *ctx)
 {
 	return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST);
@@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = {
 
 static int it66121_probe(struct i2c_client *client)
 {
-	u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
 	int ret;
 	struct it66121_ctx *ctx;
 	struct device *dev = &client->dev;
@@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client)
 	if (IS_ERR(ctx->regmap))
 		return PTR_ERR(ctx->regmap);
 
-	regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]);
-	regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]);
-	regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]);
-	regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]);
-
-	/* Revision is shared with DEVICE_ID1 */
-	revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]);
-	device_ids[1] &= IT66121_DEVICE_ID1_MASK;
+	ret = it66121_read_chip_id(ctx, false);
+	if (ret)
+		return ret;
 
-	if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid ||
-	    (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) {
+	if (ctx->vender_id != ctx->info->vid ||
+	    ctx->device_id != ctx->info->pid)
 		return -ENODEV;
-	}
 
 	ctx->bridge.funcs = &it66121_bridge_funcs;
 	ctx->bridge.of_node = dev->of_node;
@@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client)
 
 	drm_bridge_add(&ctx->bridge);
 
-	dev_info(dev, "IT66121 revision %d probed\n", revision_id);
+	dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
+		 ctx->vender_id, ctx->device_id, ctx->revision);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH 5/8] drm/bridge: it66121: Add a helper function to read chip id

[Dmitry Baryshkov]

On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> Read the required chip id data back by calling regmap_bulk_read() once,
> reduce the number of local variables needed in it66121_probe() function.
> And store its values into struct it66121_ctx, as it will be used latter.
>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 7e473beefc79..f36d05331f25 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -313,6 +313,9 @@ struct it66121_ctx {
>                 bool auto_cts;
>         } audio;
>         const struct it66121_chip_info *info;
> +       u16 vender_id;
> +       u16 device_id;
> +       u8 revision;

There is no need to store them, they are not used by the driver anywhere.

>  };
>
>  static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge)
> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx)
>         gpiod_set_value(ctx->gpio_reset, 0);
>  }
>
> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose)
> +{
> +       u8 id[4];
> +       int ret;
> +
> +       ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4);
> +       if (ret < 0) {
> +               dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ctx->vender_id = (u16)id[1] << 8 | id[0];
> +       ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]);
> +       /* Revision is shared with DEVICE_ID1 */
> +       ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]);
> +
> +       if (verbose) {
> +               dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n",
> +                        ctx->vender_id, ctx->device_id, ctx->revision);
> +       }
> +
> +       return 0;
> +}
> +
>  static inline int it66121_preamble_ddc(struct it66121_ctx *ctx)
>  {
>         return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST);
> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = {
>
>  static int it66121_probe(struct i2c_client *client)
>  {
> -       u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
>         int ret;
>         struct it66121_ctx *ctx;
>         struct device *dev = &client->dev;
> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client)
>         if (IS_ERR(ctx->regmap))
>                 return PTR_ERR(ctx->regmap);
>
> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]);
> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]);
> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]);
> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]);
> -
> -       /* Revision is shared with DEVICE_ID1 */
> -       revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]);
> -       device_ids[1] &= IT66121_DEVICE_ID1_MASK;
> +       ret = it66121_read_chip_id(ctx, false);
> +       if (ret)
> +               return ret;
>
> -       if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid ||
> -           (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) {
> +       if (ctx->vender_id != ctx->info->vid ||
> +           ctx->device_id != ctx->info->pid)
>                 return -ENODEV;
> -       }
>
>         ctx->bridge.funcs = &it66121_bridge_funcs;
>         ctx->bridge.of_node = dev->of_node;
> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client)
>
>         drm_bridge_add(&ctx->bridge);
>
> -       dev_info(dev, "IT66121 revision %d probed\n", revision_id);
> +       dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
> +                ctx->vender_id, ctx->device_id, ctx->revision);
>
>         return 0;
>  }
> --
> 2.34.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 5/8] drm/bridge: it66121: Add a helper function to read chip id

[Sui Jingfeng]

Hi,


On 2023/11/15 00:06, Dmitry Baryshkov wrote:
> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> Read the required chip id data back by calling regmap_bulk_read() once,
>> reduce the number of local variables needed in it66121_probe() function.
>> And store its values into struct it66121_ctx, as it will be used latter.
>>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++--------
>>   1 file changed, 34 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
>> index 7e473beefc79..f36d05331f25 100644
>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
>> @@ -313,6 +313,9 @@ struct it66121_ctx {
>>                  bool auto_cts;
>>          } audio;
>>          const struct it66121_chip_info *info;
>> +       u16 vender_id;
>> +       u16 device_id;
>> +       u8 revision;
> There is no need to store them, they are not used by the driver anywhere.
>
>>   };
>>
>>   static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge)
>> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx)
>>          gpiod_set_value(ctx->gpio_reset, 0);
>>   }
>>
>> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose)
>> +{
>> +       u8 id[4];
>> +       int ret;
>> +
>> +       ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4);
>> +       if (ret < 0) {
>> +               dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret);
>> +               return ret;
>> +       }
>> +
>> +       ctx->vender_id = (u16)id[1] << 8 | id[0];
>> +       ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]);
>> +       /* Revision is shared with DEVICE_ID1 */
>> +       ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]);
>> +
>> +       if (verbose) {
>> +               dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n",
>> +                        ctx->vender_id, ctx->device_id, ctx->revision);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   static inline int it66121_preamble_ddc(struct it66121_ctx *ctx)
>>   {
>>          return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST);
>> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = {
>>
>>   static int it66121_probe(struct i2c_client *client)
>>   {
>> -       u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
>>          int ret;
>>          struct it66121_ctx *ctx;
>>          struct device *dev = &client->dev;
>> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client)
>>          if (IS_ERR(ctx->regmap))
>>                  return PTR_ERR(ctx->regmap);
>>
>> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]);
>> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]);
>> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]);
>> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]);
>> -
>> -       /* Revision is shared with DEVICE_ID1 */
>> -       revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]);
>> -       device_ids[1] &= IT66121_DEVICE_ID1_MASK;
>> +       ret = it66121_read_chip_id(ctx, false);
>> +       if (ret)
>> +               return ret;
>>
>> -       if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid ||
>> -           (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) {
>> +       if (ctx->vender_id != ctx->info->vid ||
>> +           ctx->device_id != ctx->info->pid)

Q: There is no need to store them, they are not used by the driver anywhere.

A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data.


Since it6610 was introduced, this is used for chip identifying.
It can also be used with in debugfs context, to show who I am.


>>                  return -ENODEV;
>> -       }
>>
>>          ctx->bridge.funcs = &it66121_bridge_funcs;
>>          ctx->bridge.of_node = dev->of_node;
>> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client)
>>
>>          drm_bridge_add(&ctx->bridge);
>>
>> -       dev_info(dev, "IT66121 revision %d probed\n", revision_id);
>> +       dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
>> +                ctx->vender_id, ctx->device_id, ctx->revision);
>>
>>          return 0;
>>   }
>> --
>> 2.34.1
>>
>

Re: [PATCH 5/8] drm/bridge: it66121: Add a helper function to read chip id

[Dmitry Baryshkov]

On Thu, 16 Nov 2023 at 14:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Hi,
>
>
> On 2023/11/15 00:06, Dmitry Baryshkov wrote:
> > On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> >> From: Sui Jingfeng <suijingfeng@loongson.cn>
> >>
> >> Read the required chip id data back by calling regmap_bulk_read() once,
> >> reduce the number of local variables needed in it66121_probe() function.
> >> And store its values into struct it66121_ctx, as it will be used latter.
> >>
> >> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> >> ---
> >>   drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++--------
> >>   1 file changed, 34 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> >> index 7e473beefc79..f36d05331f25 100644
> >> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> >> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> >> @@ -313,6 +313,9 @@ struct it66121_ctx {
> >>                  bool auto_cts;
> >>          } audio;
> >>          const struct it66121_chip_info *info;
> >> +       u16 vender_id;
> >> +       u16 device_id;
> >> +       u8 revision;
> > There is no need to store them, they are not used by the driver anywhere.
> >
> >>   };
> >>
> >>   static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge)
> >> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx)
> >>          gpiod_set_value(ctx->gpio_reset, 0);
> >>   }
> >>
> >> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose)
> >> +{
> >> +       u8 id[4];
> >> +       int ret;
> >> +
> >> +       ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4);
> >> +       if (ret < 0) {
> >> +               dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret);
> >> +               return ret;
> >> +       }
> >> +
> >> +       ctx->vender_id = (u16)id[1] << 8 | id[0];
> >> +       ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]);
> >> +       /* Revision is shared with DEVICE_ID1 */
> >> +       ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]);
> >> +
> >> +       if (verbose) {
> >> +               dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n",
> >> +                        ctx->vender_id, ctx->device_id, ctx->revision);
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >>   static inline int it66121_preamble_ddc(struct it66121_ctx *ctx)
> >>   {
> >>          return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST);
> >> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = {
> >>
> >>   static int it66121_probe(struct i2c_client *client)
> >>   {
> >> -       u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
> >>          int ret;
> >>          struct it66121_ctx *ctx;
> >>          struct device *dev = &client->dev;
> >> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client)
> >>          if (IS_ERR(ctx->regmap))
> >>                  return PTR_ERR(ctx->regmap);
> >>
> >> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]);
> >> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]);
> >> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]);
> >> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]);
> >> -
> >> -       /* Revision is shared with DEVICE_ID1 */
> >> -       revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]);
> >> -       device_ids[1] &= IT66121_DEVICE_ID1_MASK;
> >> +       ret = it66121_read_chip_id(ctx, false);
> >> +       if (ret)
> >> +               return ret;
> >>
> >> -       if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid ||
> >> -           (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) {
> >> +       if (ctx->vender_id != ctx->info->vid ||
> >> +           ctx->device_id != ctx->info->pid)
>
> Q: There is no need to store them, they are not used by the driver anywhere.
>
> A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data.

And the patch 7 will be changed once you have proper i2c client struct
registered.

>
>
> Since it6610 was introduced, this is used for chip identifying.
> It can also be used with in debugfs context, to show who I am.

I'd say, there is little point in whoami debugfs files. Debugfs is for
the useful information.

>
>
> >>                  return -ENODEV;
> >> -       }
> >>
> >>          ctx->bridge.funcs = &it66121_bridge_funcs;
> >>          ctx->bridge.of_node = dev->of_node;
> >> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client)
> >>
> >>          drm_bridge_add(&ctx->bridge);
> >>
> >> -       dev_info(dev, "IT66121 revision %d probed\n", revision_id);
> >> +       dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
> >> +                ctx->vender_id, ctx->device_id, ctx->revision);
> >>
> >>          return 0;
> >>   }
> >> --
> >> 2.34.1
> >>
> >



-- 
With best wishes
Dmitry

Re: [PATCH 5/8] drm/bridge: it66121: Add a helper function to read chip id

[Sui Jingfeng]

Hi,


On 2023/11/16 21:00, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 14:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>> Hi,
>>
>>
>> On 2023/11/15 00:06, Dmitry Baryshkov wrote:
>>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>
>>>> Read the required chip id data back by calling regmap_bulk_read() once,
>>>> reduce the number of local variables needed in it66121_probe() function.
>>>> And store its values into struct it66121_ctx, as it will be used latter.
>>>>
>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> ---
>>>>    drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++--------
>>>>    1 file changed, 34 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
>>>> index 7e473beefc79..f36d05331f25 100644
>>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
>>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
>>>> @@ -313,6 +313,9 @@ struct it66121_ctx {
>>>>                   bool auto_cts;
>>>>           } audio;
>>>>           const struct it66121_chip_info *info;
>>>> +       u16 vender_id;
>>>> +       u16 device_id;
>>>> +       u8 revision;
>>> There is no need to store them, they are not used by the driver anywhere.
>>>
>>>>    };
>>>>
>>>>    static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge)
>>>> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx)
>>>>           gpiod_set_value(ctx->gpio_reset, 0);
>>>>    }
>>>>
>>>> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose)
>>>> +{
>>>> +       u8 id[4];
>>>> +       int ret;
>>>> +
>>>> +       ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4);
>>>> +       if (ret < 0) {
>>>> +               dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       ctx->vender_id = (u16)id[1] << 8 | id[0];
>>>> +       ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]);
>>>> +       /* Revision is shared with DEVICE_ID1 */
>>>> +       ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]);
>>>> +
>>>> +       if (verbose) {
>>>> +               dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n",
>>>> +                        ctx->vender_id, ctx->device_id, ctx->revision);
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>    static inline int it66121_preamble_ddc(struct it66121_ctx *ctx)
>>>>    {
>>>>           return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST);
>>>> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = {
>>>>
>>>>    static int it66121_probe(struct i2c_client *client)
>>>>    {
>>>> -       u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
>>>>           int ret;
>>>>           struct it66121_ctx *ctx;
>>>>           struct device *dev = &client->dev;
>>>> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client)
>>>>           if (IS_ERR(ctx->regmap))
>>>>                   return PTR_ERR(ctx->regmap);
>>>>
>>>> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]);
>>>> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]);
>>>> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]);
>>>> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]);
>>>> -
>>>> -       /* Revision is shared with DEVICE_ID1 */
>>>> -       revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]);
>>>> -       device_ids[1] &= IT66121_DEVICE_ID1_MASK;
>>>> +       ret = it66121_read_chip_id(ctx, false);
>>>> +       if (ret)
>>>> +               return ret;
>>>>
>>>> -       if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid ||
>>>> -           (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) {
>>>> +       if (ctx->vender_id != ctx->info->vid ||
>>>> +           ctx->device_id != ctx->info->pid)
>> Q: There is no need to store them, they are not used by the driver anywhere.
>>
>> A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data.
> And the patch 7 will be changed once you have proper i2c client struct
> registered.
>
We will create a "wrong"i2c client intentionally by calling i2c_new_client_device, the "wrong" 
here means that 'board_info.type'  is not equal to "it66121" or "it6610".

I probably know what you means, but please note that we are not object you at here.
The reason why we prefer theit66121_get_match_data() over the i2c_get_match_data() is that our 
it66121_get_match_data()  works with *the least dependency*. Ourversion
don't rely on the DT to provide compatible strings. Because the it66121 itself already
provided constant hard-coded vender id and device id on-chip register. This is a bit
similar with the PCIe devices. This chip id speak everything about chip identify.
By re-arranging the variants into array, searching and matching against became
straight-forward. We can also utilize the vender id and device id information to
dynamic bind the instance specific functions *at the setup(probe) time*. Which helps
to untangle the it66121 and it6610 cases with function object. Currently they are
tangled. This is not a problem simply because the model supported is too small(less).


>>
>> Since it6610 was introduced, this is used for chip identifying.
>> It can also be used with in debugfs context, to show who I am.
> I'd say, there is little point in whoami debugfs files. Debugfs is for
> the useful information.
>
>>
>>>>                   return -ENODEV;
>>>> -       }
>>>>
>>>>           ctx->bridge.funcs = &it66121_bridge_funcs;
>>>>           ctx->bridge.of_node = dev->of_node;
>>>> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client)
>>>>
>>>>           drm_bridge_add(&ctx->bridge);
>>>>
>>>> -       dev_info(dev, "IT66121 revision %d probed\n", revision_id);
>>>> +       dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
>>>> +                ctx->vender_id, ctx->device_id, ctx->revision);
>>>>
>>>>           return 0;
>>>>    }
>>>> --
>>>> 2.34.1
>>>>
>
>

Re: [PATCH 5/8] drm/bridge: it66121: Add a helper function to read chip id

[Dmitry Baryshkov]

On Thu, 16 Nov 2023 at 20:29, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Hi,
>
>
> On 2023/11/16 21:00, Dmitry Baryshkov wrote:
> > On Thu, 16 Nov 2023 at 14:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> >> Hi,
> >>
> >>
> >> On 2023/11/15 00:06, Dmitry Baryshkov wrote:
> >>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> >>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
> >>>>
> >>>> Read the required chip id data back by calling regmap_bulk_read() once,
> >>>> reduce the number of local variables needed in it66121_probe() function.
> >>>> And store its values into struct it66121_ctx, as it will be used latter.
> >>>>
> >>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> >>>> ---
> >>>>    drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++--------
> >>>>    1 file changed, 34 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> >>>> index 7e473beefc79..f36d05331f25 100644
> >>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> >>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> >>>> @@ -313,6 +313,9 @@ struct it66121_ctx {
> >>>>                   bool auto_cts;
> >>>>           } audio;
> >>>>           const struct it66121_chip_info *info;
> >>>> +       u16 vender_id;
> >>>> +       u16 device_id;
> >>>> +       u8 revision;
> >>> There is no need to store them, they are not used by the driver anywhere.
> >>>
> >>>>    };
> >>>>
> >>>>    static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge)
> >>>> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx)
> >>>>           gpiod_set_value(ctx->gpio_reset, 0);
> >>>>    }
> >>>>
> >>>> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose)
> >>>> +{
> >>>> +       u8 id[4];
> >>>> +       int ret;
> >>>> +
> >>>> +       ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4);
> >>>> +       if (ret < 0) {
> >>>> +               dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret);
> >>>> +               return ret;
> >>>> +       }
> >>>> +
> >>>> +       ctx->vender_id = (u16)id[1] << 8 | id[0];
> >>>> +       ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]);
> >>>> +       /* Revision is shared with DEVICE_ID1 */
> >>>> +       ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]);
> >>>> +
> >>>> +       if (verbose) {
> >>>> +               dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n",
> >>>> +                        ctx->vender_id, ctx->device_id, ctx->revision);
> >>>> +       }
> >>>> +
> >>>> +       return 0;
> >>>> +}
> >>>> +
> >>>>    static inline int it66121_preamble_ddc(struct it66121_ctx *ctx)
> >>>>    {
> >>>>           return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST);
> >>>> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = {
> >>>>
> >>>>    static int it66121_probe(struct i2c_client *client)
> >>>>    {
> >>>> -       u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
> >>>>           int ret;
> >>>>           struct it66121_ctx *ctx;
> >>>>           struct device *dev = &client->dev;
> >>>> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client)
> >>>>           if (IS_ERR(ctx->regmap))
> >>>>                   return PTR_ERR(ctx->regmap);
> >>>>
> >>>> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]);
> >>>> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]);
> >>>> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]);
> >>>> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]);
> >>>> -
> >>>> -       /* Revision is shared with DEVICE_ID1 */
> >>>> -       revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]);
> >>>> -       device_ids[1] &= IT66121_DEVICE_ID1_MASK;
> >>>> +       ret = it66121_read_chip_id(ctx, false);
> >>>> +       if (ret)
> >>>> +               return ret;
> >>>>
> >>>> -       if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid ||
> >>>> -           (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) {
> >>>> +       if (ctx->vender_id != ctx->info->vid ||
> >>>> +           ctx->device_id != ctx->info->pid)
> >> Q: There is no need to store them, they are not used by the driver anywhere.
> >>
> >> A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data.
> > And the patch 7 will be changed once you have proper i2c client struct
> > registered.
> >
> We will create a "wrong"i2c client intentionally by calling i2c_new_client_device, the "wrong"
> here means that 'board_info.type'  is not equal to "it66121" or "it6610".
>
> I probably know what you means, but please note that we are not object you at here.
> The reason why we prefer theit66121_get_match_data() over the i2c_get_match_data() is that our
> it66121_get_match_data()  works with *the least dependency*. Ourversion
> don't rely on the DT to provide compatible strings. Because the it66121 itself already
> provided constant hard-coded vender id and device id on-chip register. This is a bit
> similar with the PCIe devices. This chip id speak everything about chip identify.
> By re-arranging the variants into array, searching and matching against became
> straight-forward. We can also utilize the vender id and device id information to
> dynamic bind the instance specific functions *at the setup(probe) time*. Which helps
> to untangle the it66121 and it6610 cases with function object. Currently they are
> tangled. This is not a problem simply because the model supported is too small(less).

The difference is that PCI or USB are discoverable buses, but I2C and
SPI are not.
The kernel tends to use generic mechanisms if possible. And for the
I2C / SPI buses the generic mechanism is match data.
What you are doing is nice if there was just the it66121 driver.
However there is more than that. Many more drivers. And the generic
approach becomes important, as it helps other developers to work with
better efficiency.

> >> Since it6610 was introduced, this is used for chip identifying.
> >> It can also be used with in debugfs context, to show who I am.
> > I'd say, there is little point in whoami debugfs files. Debugfs is for
> > the useful information.
> >
> >>
> >>>>                   return -ENODEV;
> >>>> -       }
> >>>>
> >>>>           ctx->bridge.funcs = &it66121_bridge_funcs;
> >>>>           ctx->bridge.of_node = dev->of_node;
> >>>> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client)
> >>>>
> >>>>           drm_bridge_add(&ctx->bridge);
> >>>>
> >>>> -       dev_info(dev, "IT66121 revision %d probed\n", revision_id);
> >>>> +       dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
> >>>> +                ctx->vender_id, ctx->device_id, ctx->revision);
> >>>>
> >>>>           return 0;
> >>>>    }
> >>>> --
> >>>> 2.34.1
> >>>>
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH 5/8] drm/bridge: it66121: Add a helper function to read chip id

[Sui Jingfeng]

Hi,


On 2023/11/16 21:00, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 14:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>> Hi,
>>
>>
>> On 2023/11/15 00:06, Dmitry Baryshkov wrote:
>>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>
>>>> Read the required chip id data back by calling regmap_bulk_read() once,
>>>> reduce the number of local variables needed in it66121_probe() function.
>>>> And store its values into struct it66121_ctx, as it will be used latter.
>>>>
>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> ---
>>>>    drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++--------
>>>>    1 file changed, 34 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
>>>> index 7e473beefc79..f36d05331f25 100644
>>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
>>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
>>>> @@ -313,6 +313,9 @@ struct it66121_ctx {
>>>>                   bool auto_cts;
>>>>           } audio;
>>>>           const struct it66121_chip_info *info;
>>>> +       u16 vender_id;
>>>> +       u16 device_id;
>>>> +       u8 revision;
>>> There is no need to store them, they are not used by the driver anywhere.
>>>
>>>>    };
>>>>
>>>>    static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge)
>>>> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx)
>>>>           gpiod_set_value(ctx->gpio_reset, 0);
>>>>    }
>>>>
>>>> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose)
>>>> +{
>>>> +       u8 id[4];
>>>> +       int ret;
>>>> +
>>>> +       ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4);
>>>> +       if (ret < 0) {
>>>> +               dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       ctx->vender_id = (u16)id[1] << 8 | id[0];
>>>> +       ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]);
>>>> +       /* Revision is shared with DEVICE_ID1 */
>>>> +       ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]);
>>>> +
>>>> +       if (verbose) {
>>>> +               dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n",
>>>> +                        ctx->vender_id, ctx->device_id, ctx->revision);
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>    static inline int it66121_preamble_ddc(struct it66121_ctx *ctx)
>>>>    {
>>>>           return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST);
>>>> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = {
>>>>
>>>>    static int it66121_probe(struct i2c_client *client)
>>>>    {
>>>> -       u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
>>>>           int ret;
>>>>           struct it66121_ctx *ctx;
>>>>           struct device *dev = &client->dev;
>>>> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client)
>>>>           if (IS_ERR(ctx->regmap))
>>>>                   return PTR_ERR(ctx->regmap);
>>>>
>>>> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]);
>>>> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]);
>>>> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]);
>>>> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]);
>>>> -
>>>> -       /* Revision is shared with DEVICE_ID1 */
>>>> -       revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]);
>>>> -       device_ids[1] &= IT66121_DEVICE_ID1_MASK;
>>>> +       ret = it66121_read_chip_id(ctx, false);
>>>> +       if (ret)
>>>> +               return ret;
>>>>
>>>> -       if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid ||
>>>> -           (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) {
>>>> +       if (ctx->vender_id != ctx->info->vid ||
>>>> +           ctx->device_id != ctx->info->pid)
>> Q: There is no need to store them, they are not used by the driver anywhere.
>>
>> A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data.
> And the patch 7 will be changed once you have proper i2c client struct
> registered.
>
>>
>> Since it6610 was introduced, this is used for chip identifying.
>> It can also be used with in debugfs context, to show who I am.
> I'd say, there is little point in whoami debugfs files. Debugfs is for
> the useful information.

Sinceit6610 was introduced, how do you know what the device it66121 driver is 
binding? Printing model specific information is common practice for a 
large driver. Especially if you can only able to debug remotely where 
only a SSH is given. You could see debugfs of drm/etnaviv for a 
reference. It is common to testing a large driver running on 20+ 
machines with various hardware model.


>>
>>>>                   return -ENODEV;
>>>> -       }
>>>>
>>>>           ctx->bridge.funcs = &it66121_bridge_funcs;
>>>>           ctx->bridge.of_node = dev->of_node;
>>>> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client)
>>>>
>>>>           drm_bridge_add(&ctx->bridge);
>>>>
>>>> -       dev_info(dev, "IT66121 revision %d probed\n", revision_id);
>>>> +       dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
>>>> +                ctx->vender_id, ctx->device_id, ctx->revision);
>>>>
>>>>           return 0;
>>>>    }
>>>> --
>>>> 2.34.1
>>>>
>
>

Re: [PATCH 5/8] drm/bridge: it66121: Add a helper function to read chip id

[Dmitry Baryshkov]

On Thu, 23 Nov 2023 at 07:37, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Hi,
>
>
> On 2023/11/16 21:00, Dmitry Baryshkov wrote:
> > On Thu, 16 Nov 2023 at 14:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> >> Hi,
> >>
> >>
> >> On 2023/11/15 00:06, Dmitry Baryshkov wrote:
> >>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> >>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
> >>>>
> >>>> Read the required chip id data back by calling regmap_bulk_read() once,
> >>>> reduce the number of local variables needed in it66121_probe() function.
> >>>> And store its values into struct it66121_ctx, as it will be used latter.
> >>>>
> >>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> >>>> ---
> >>>>    drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++--------
> >>>>    1 file changed, 34 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> >>>> index 7e473beefc79..f36d05331f25 100644
> >>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> >>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> >>>> @@ -313,6 +313,9 @@ struct it66121_ctx {
> >>>>                   bool auto_cts;
> >>>>           } audio;
> >>>>           const struct it66121_chip_info *info;
> >>>> +       u16 vender_id;
> >>>> +       u16 device_id;
> >>>> +       u8 revision;
> >>> There is no need to store them, they are not used by the driver anywhere.
> >>>
> >>>>    };
> >>>>
> >>>>    static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge)
> >>>> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx)
> >>>>           gpiod_set_value(ctx->gpio_reset, 0);
> >>>>    }
> >>>>
> >>>> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose)
> >>>> +{
> >>>> +       u8 id[4];
> >>>> +       int ret;
> >>>> +
> >>>> +       ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4);
> >>>> +       if (ret < 0) {
> >>>> +               dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret);
> >>>> +               return ret;
> >>>> +       }
> >>>> +
> >>>> +       ctx->vender_id = (u16)id[1] << 8 | id[0];
> >>>> +       ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]);
> >>>> +       /* Revision is shared with DEVICE_ID1 */
> >>>> +       ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]);
> >>>> +
> >>>> +       if (verbose) {
> >>>> +               dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n",
> >>>> +                        ctx->vender_id, ctx->device_id, ctx->revision);
> >>>> +       }
> >>>> +
> >>>> +       return 0;
> >>>> +}
> >>>> +
> >>>>    static inline int it66121_preamble_ddc(struct it66121_ctx *ctx)
> >>>>    {
> >>>>           return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST);
> >>>> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = {
> >>>>
> >>>>    static int it66121_probe(struct i2c_client *client)
> >>>>    {
> >>>> -       u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
> >>>>           int ret;
> >>>>           struct it66121_ctx *ctx;
> >>>>           struct device *dev = &client->dev;
> >>>> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client)
> >>>>           if (IS_ERR(ctx->regmap))
> >>>>                   return PTR_ERR(ctx->regmap);
> >>>>
> >>>> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]);
> >>>> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]);
> >>>> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]);
> >>>> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]);
> >>>> -
> >>>> -       /* Revision is shared with DEVICE_ID1 */
> >>>> -       revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]);
> >>>> -       device_ids[1] &= IT66121_DEVICE_ID1_MASK;
> >>>> +       ret = it66121_read_chip_id(ctx, false);
> >>>> +       if (ret)
> >>>> +               return ret;
> >>>>
> >>>> -       if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid ||
> >>>> -           (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) {
> >>>> +       if (ctx->vender_id != ctx->info->vid ||
> >>>> +           ctx->device_id != ctx->info->pid)
> >> Q: There is no need to store them, they are not used by the driver anywhere.
> >>
> >> A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data.
> > And the patch 7 will be changed once you have proper i2c client struct
> > registered.
> >
> >>
> >> Since it6610 was introduced, this is used for chip identifying.
> >> It can also be used with in debugfs context, to show who I am.
> > I'd say, there is little point in whoami debugfs files. Debugfs is for
> > the useful information.
>
> Sinceit6610 was introduced, how do you know what the device it66121 driver is
> binding? Printing model specific information is common practice for a
> large driver. Especially if you can only able to debug remotely where
> only a SSH is given.

cat /sys/bus/i2c/devices/.../name

> You could see debugfs of drm/etnaviv for a
> reference. It is common to testing a large driver running on 20+
> machines with various hardware model.
>
>
> >>
> >>>>                   return -ENODEV;
> >>>> -       }
> >>>>
> >>>>           ctx->bridge.funcs = &it66121_bridge_funcs;
> >>>>           ctx->bridge.of_node = dev->of_node;
> >>>> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client)
> >>>>
> >>>>           drm_bridge_add(&ctx->bridge);
> >>>>
> >>>> -       dev_info(dev, "IT66121 revision %d probed\n", revision_id);
> >>>> +       dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
> >>>> +                ctx->vender_id, ctx->device_id, ctx->revision);
> >>>>
> >>>>           return 0;
> >>>>    }
> >>>> --
> >>>> 2.34.1
> >>>>
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH 5/8] drm/bridge: it66121: Add a helper function to read chip id

[Sui Jingfeng]

Hi,


On 2023/11/23 15:48, Dmitry Baryshkov wrote:
> On Thu, 23 Nov 2023 at 07:37, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>> Hi,
>>
>>
>> On 2023/11/16 21:00, Dmitry Baryshkov wrote:
>>> On Thu, 16 Nov 2023 at 14:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 2023/11/15 00:06, Dmitry Baryshkov wrote:
>>>>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>>
>>>>>> Read the required chip id data back by calling regmap_bulk_read() once,
>>>>>> reduce the number of local variables needed in it66121_probe() function.
>>>>>> And store its values into struct it66121_ctx, as it will be used latter.
>>>>>>
>>>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>> ---
>>>>>>     drivers/gpu/drm/bridge/ite-it66121.c | 47 ++++++++++++++++++++--------
>>>>>>     1 file changed, 34 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>> index 7e473beefc79..f36d05331f25 100644
>>>>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>> @@ -313,6 +313,9 @@ struct it66121_ctx {
>>>>>>                    bool auto_cts;
>>>>>>            } audio;
>>>>>>            const struct it66121_chip_info *info;
>>>>>> +       u16 vender_id;
>>>>>> +       u16 device_id;
>>>>>> +       u8 revision;
>>>>> There is no need to store them, they are not used by the driver anywhere.
>>>>>
>>>>>>     };
>>>>>>
>>>>>>     static inline struct it66121_ctx *bridge_to_it66121(struct drm_bridge *bridge)
>>>>>> @@ -399,6 +402,30 @@ static void it66121_hw_reset(struct it66121_ctx *ctx)
>>>>>>            gpiod_set_value(ctx->gpio_reset, 0);
>>>>>>     }
>>>>>>
>>>>>> +static int it66121_read_chip_id(struct it66121_ctx *ctx, bool verbose)
>>>>>> +{
>>>>>> +       u8 id[4];
>>>>>> +       int ret;
>>>>>> +
>>>>>> +       ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG, id, 4);
>>>>>> +       if (ret < 0) {
>>>>>> +               dev_err(ctx->dev, "Failed to read chip ID: %d\n", ret);
>>>>>> +               return ret;
>>>>>> +       }
>>>>>> +
>>>>>> +       ctx->vender_id = (u16)id[1] << 8 | id[0];
>>>>>> +       ctx->device_id = ((u16)(id[3] & IT66121_DEVICE_ID1_MASK) << 8 | id[2]);
>>>>>> +       /* Revision is shared with DEVICE_ID1 */
>>>>>> +       ctx->revision = FIELD_GET(IT66121_REVISION_MASK, id[3]);
>>>>>> +
>>>>>> +       if (verbose) {
>>>>>> +               dev_info(ctx->dev, "Found ITE66121: 0x%x%x, revision: %u\n",
>>>>>> +                        ctx->vender_id, ctx->device_id, ctx->revision);
>>>>>> +       }
>>>>>> +
>>>>>> +       return 0;
>>>>>> +}
>>>>>> +
>>>>>>     static inline int it66121_preamble_ddc(struct it66121_ctx *ctx)
>>>>>>     {
>>>>>>            return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST);
>>>>>> @@ -1561,7 +1588,6 @@ static const char * const it66121_supplies[] = {
>>>>>>
>>>>>>     static int it66121_probe(struct i2c_client *client)
>>>>>>     {
>>>>>> -       u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
>>>>>>            int ret;
>>>>>>            struct it66121_ctx *ctx;
>>>>>>            struct device *dev = &client->dev;
>>>>>> @@ -1603,19 +1629,13 @@ static int it66121_probe(struct i2c_client *client)
>>>>>>            if (IS_ERR(ctx->regmap))
>>>>>>                    return PTR_ERR(ctx->regmap);
>>>>>>
>>>>>> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]);
>>>>>> -       regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]);
>>>>>> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]);
>>>>>> -       regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]);
>>>>>> -
>>>>>> -       /* Revision is shared with DEVICE_ID1 */
>>>>>> -       revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]);
>>>>>> -       device_ids[1] &= IT66121_DEVICE_ID1_MASK;
>>>>>> +       ret = it66121_read_chip_id(ctx, false);
>>>>>> +       if (ret)
>>>>>> +               return ret;
>>>>>>
>>>>>> -       if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid ||
>>>>>> -           (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) {
>>>>>> +       if (ctx->vender_id != ctx->info->vid ||
>>>>>> +           ctx->device_id != ctx->info->pid)
>>>> Q: There is no need to store them, they are not used by the driver anywhere.
>>>>
>>>> A: Here it is used, it is also used by the 0007-patch to get the entity(instance)-specific data.
>>> And the patch 7 will be changed once you have proper i2c client struct
>>> registered.
>>>
>>>> Since it6610 was introduced, this is used for chip identifying.
>>>> It can also be used with in debugfs context, to show who I am.
>>> I'd say, there is little point in whoami debugfs files. Debugfs is for
>>> the useful information.
>> Sinceit6610 was introduced, how do you know what the device it66121 driver is
>> binding? Printing model specific information is common practice for a
>> large driver. Especially if you can only able to debug remotely where
>> only a SSH is given.
> cat /sys/bus/i2c/devices/.../name

No, this is not enough. This is also not straight-forward.
Because the system will have multiple i2c device. This is
a bit hard to find which one is corresponding to the it66121.

This is needed during the development and debug phase.
We will also want to know the vendor id, device id and revision id.
As those chip id are constant values, matching those chip id against
the values in datasheet is a kind of sanity test. Which helps us to
know if the i2c regmap(the communication channel) has been setup
successfully or not.


>> You could see debugfs of drm/etnaviv for a
>> reference. It is common to testing a large driver running on 20+
>> machines with various hardware model.
>>
>>
>>>>>>                    return -ENODEV;
>>>>>> -       }
>>>>>>
>>>>>>            ctx->bridge.funcs = &it66121_bridge_funcs;
>>>>>>            ctx->bridge.of_node = dev->of_node;
>>>>>> @@ -1633,7 +1653,8 @@ static int it66121_probe(struct i2c_client *client)
>>>>>>
>>>>>>            drm_bridge_add(&ctx->bridge);
>>>>>>
>>>>>> -       dev_info(dev, "IT66121 revision %d probed\n", revision_id);
>>>>>> +       dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
>>>>>> +                ctx->vender_id, ctx->device_id, ctx->revision);
>>>>>>
>>>>>>            return 0;
>>>>>>     }
>>>>>> --
>>>>>> 2.34.1
>>>>>>
>>>
>
>

[PATCH 6/8] drm/bridge: it66121: Add a helper to initialize the DRM bridge structure

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Helps the it66121_probe() function to reduce weight, no functional change.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index f36d05331f25..2f7f00f1bedb 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -1004,6 +1004,21 @@ static const struct drm_bridge_funcs it66121_bridge_funcs = {
 	.hpd_disable = it66121_bridge_hpd_disable,
 };
 
+static void it66121_bridge_init_base(struct drm_bridge *bridge,
+				     struct device_node *of_node,
+				     bool hpd_support)
+{
+	bridge->funcs = &it66121_bridge_funcs;
+	bridge->type = DRM_MODE_CONNECTOR_HDMIA;
+	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
+	if (hpd_support)
+		bridge->ops |= DRM_BRIDGE_OP_HPD;
+
+	bridge->of_node = of_node;
+
+	drm_bridge_add(bridge);
+}
+
 static irqreturn_t it66121_irq_threaded_handler(int irq, void *dev_id)
 {
 	int ret;
@@ -1637,11 +1652,6 @@ static int it66121_probe(struct i2c_client *client)
 	    ctx->device_id != ctx->info->pid)
 		return -ENODEV;
 
-	ctx->bridge.funcs = &it66121_bridge_funcs;
-	ctx->bridge.of_node = dev->of_node;
-	ctx->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
-	ctx->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
-
 	ret = devm_request_threaded_irq(dev, client->irq, NULL,	it66121_irq_threaded_handler,
 					IRQF_ONESHOT, dev_name(dev), ctx);
 	if (ret < 0) {
@@ -1649,9 +1659,9 @@ static int it66121_probe(struct i2c_client *client)
 		return ret;
 	}
 
-	it66121_audio_codec_init(ctx, dev);
+	it66121_bridge_init_base(&ctx->bridge, dev->of_node, true);
 
-	drm_bridge_add(&ctx->bridge);
+	it66121_audio_codec_init(ctx, dev);
 
 	dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
 		 ctx->vender_id, ctx->device_id, ctx->revision);
-- 
2.34.1

Re: [PATCH 6/8] drm/bridge: it66121: Add a helper to initialize the DRM bridge structure

[Dmitry Baryshkov]

On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> Helps the it66121_probe() function to reduce weight, no functional change.

This is not a proper commit message.

>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index f36d05331f25..2f7f00f1bedb 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -1004,6 +1004,21 @@ static const struct drm_bridge_funcs it66121_bridge_funcs = {
>         .hpd_disable = it66121_bridge_hpd_disable,
>  };
>
> +static void it66121_bridge_init_base(struct drm_bridge *bridge,
> +                                    struct device_node *of_node,
> +                                    bool hpd_support)

At this point there is no reason for the bridge to miss HPD support.

> +{
> +       bridge->funcs = &it66121_bridge_funcs;
> +       bridge->type = DRM_MODE_CONNECTOR_HDMIA;
> +       bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
> +       if (hpd_support)
> +               bridge->ops |= DRM_BRIDGE_OP_HPD;
> +
> +       bridge->of_node = of_node;
> +
> +       drm_bridge_add(bridge);
> +}
> +
>  static irqreturn_t it66121_irq_threaded_handler(int irq, void *dev_id)
>  {
>         int ret;
> @@ -1637,11 +1652,6 @@ static int it66121_probe(struct i2c_client *client)
>             ctx->device_id != ctx->info->pid)
>                 return -ENODEV;
>
> -       ctx->bridge.funcs = &it66121_bridge_funcs;
> -       ctx->bridge.of_node = dev->of_node;
> -       ctx->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> -       ctx->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
> -
>         ret = devm_request_threaded_irq(dev, client->irq, NULL, it66121_irq_threaded_handler,
>                                         IRQF_ONESHOT, dev_name(dev), ctx);
>         if (ret < 0) {
> @@ -1649,9 +1659,9 @@ static int it66121_probe(struct i2c_client *client)
>                 return ret;
>         }
>
> -       it66121_audio_codec_init(ctx, dev);
> +       it66121_bridge_init_base(&ctx->bridge, dev->of_node, true);
>
> -       drm_bridge_add(&ctx->bridge);
> +       it66121_audio_codec_init(ctx, dev);
>
>         dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
>                  ctx->vender_id, ctx->device_id, ctx->revision);
> --
> 2.34.1
>


-- 
With best wishes
Dmitry

[PATCH 7/8] drm/bridge: it66121: Add another implementation for getting match data

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Because if use the i2c_get_match_data() function on our platform, the
following error will emerge.

[    3.597872] ACPI: bus type drm_connector registered
[    3.602877] loongson 0000:00:06.1: Found LS7A1000 bridge chipset, revision: 1
[    3.610013] loongson 0000:00:06.1: [drm] dc: 264MHz, gmc: 529MHz, gpu: 529MHz
[    3.617111] loongson 0000:00:06.1: [drm] Dedicated vram start: 0xe0030000000, size: 64MiB
[    3.633628] loongson 0000:00:06.1: [drm] Loongson VBIOS version: 0.3
[    3.639944] loongson 0000:00:06.1: [drm] Loongson VBIOS: has 8 DCBs
[    3.646271] Console: switching to colour dummy device 80x25
[    3.651902] loongson 0000:00:06.1: [drm] VRAM: 4096 pages ready
[    3.657791] loongson 0000:00:06.1: [drm] GTT: 32768 pages ready
[    3.663731] loongson 0000:00:06.1: [drm] lsdc-i2c0(sda pin mask=1, scl pin mask=2) created
[    3.671966] loongson 0000:00:06.1: [drm] lsdc-i2c1(sda pin mask=4, scl pin mask=8) created
[    3.680188] loongson 0000:00:06.1: [drm] DisplayPipe-0 has DVO-0 connector
[    3.711860] debugfs: Directory '1-004c' with parent 'regmap' already present!
[    3.720312] CPU 0 Unable to handle kernel paging request at virtual address 0000000000000090, era == 9000000002faffec, ra == 9000000002faffc8
[    3.732935] Oops[#1]:
[    3.735187] CPU: 0 PID: 8 Comm: kworker/0:0 Not tainted 6.6.0-rc3+ #941
[    3.741757] Hardware name: Loongson Loongson-3A5000-7A1000-1w-ML5A/Loongson-LS3A5000-7A1000-1w-ML5A, BIOS vUDK2018-LoongArch-V4.0.05-test-stable202305 06/
[    3.755500] Workqueue: events work_for_cpu_fn
[    3.759833] pc 9000000002faffec ra 9000000002faffc8 tp 900000010017c000 sp 900000010017fb80
[    3.768132] a0 0000000000000000 a1 9000000101c00420 a2 0000000000000001 a3 900000010017fbc0
[    3.776430] a4 0000000000000004 a5 0000000000000000 a6 0000000000000001 a7 0000000000000001
[    3.784727] t0 0000000000000000 t1 0000000000000001 t2 0000000006124954 t3 0000000000000012
[    3.793026] t4 0000000000004954 t5 900000000800b530 t6 0000000000000000 t7 0000000000018500
[    3.801324] t8 90000001019c90c0 u0 9000000101c00c00 s9 fffffffffffff000 s0 9000000101c00400
[    3.809621] s1 0000000000000000 s2 0000000000000000 s3 9000000003c42cb0 s4 9000000101c00420
[    3.817920] s5 900000010017fc48 s6 0000000000000000 s7 0000000000000000 s8 90000000037fa000
[    3.826219]    ra: 9000000002faffc8 i2c_get_match_data+0x24/0xac
[    3.832189]   ERA: 9000000002faffec i2c_get_match_data+0x48/0xac
[    3.838156]  CRMD: 000000b0 (PLV0 -IE -DA +PG DACF=CC DACM=CC -WE)
[    3.844303]  PRMD: 00000004 (PPLV0 +PIE -PWE)
[    3.848631]  EUEN: 00000000 (-FPE -SXE -ASXE -BTE)
[    3.853391]  ECFG: 00071c1d (LIE=0,2-4,10-12 VS=7)
[    3.858149] ESTAT: 00010000 [PIL] (IS= ECode=1 EsubCode=0)
[    3.863599]  BADV: 0000000000000090
[    3.867058]  PRID: 0014c011 (Loongson-64bit, Loongson-3A5000)
[    3.872764] Modules linked in:
[    3.875793] Process kworker/0:0 (pid: 8, threadinfo=(____ptrval____), task=(____ptrval____))
[    3.884180] Stack : 9000000101c00400 0000000000000000 900000010177f428 9000000002ceb734
[    3.892137]         9000000101c00404 90000000037fa000 900000010017fc60 9000000101c00400
[    3.900093]         9000000016124954 8c326e68047d36df 0000004c037fa000 900000010017fc50
[    3.908050]         0000000000000080 9000000003c44490 900000010017fcb0 90000001003e9ec0
[    3.916007]         0000000000000001 900000010044f000 900000010017fc60 9000000101c00400
[    3.923964]         90000000037fa000 9000000002cf05a8 90000001003e8000 900000010017fcb2
[    3.931920]         000000304c17fc68 0000000000000000 000000000000004c 0000000000000000
[    3.939877]         4031323136365449 0000000000006334 004c000000000000 0000000000000000
[    3.947833]         0000000000000000 0000000000000000 0000000000000000 0000000000000000
[    3.955790]         0000000000000000 0000000000000000 9000000100449a98 8c326e68047d36df
[    3.963746]         ...
[    3.966170] Call Trace:
[    3.966172] [<9000000002faffec>] i2c_get_match_data+0x48/0xac
[    3.974304] [<9000000002ceb734>] it66121_create_bridge+0x15c/0x404
[    3.980446] [<9000000002cf05a8>] ls7a1000_output_init+0x228/0x248
[    3.986503] [<9000000002cee494>] lsdc_pci_probe+0x4dc/0x5b0
[    3.992038] [<9000000002b86b50>] local_pci_probe+0x48/0xa0
[    3.997487] [<90000000023b5d04>] work_for_cpu_fn+0x1c/0x30
[    4.002936] [<90000000023b7c0c>] process_one_work+0x14c/0x288
[    4.008644] [<90000000023b8084>] worker_thread+0x33c/0x438
[    4.014092] [<90000000023c1ccc>] kthread+0x124/0x130
[    4.019023] [<9000000002381188>] ret_from_kernel_thread+0xc/0xa4
[    4.024991]
[    4.026463] Code: 26000079  02c08063  4c000020 <28c24318> 43ffe31f  43ffdeff  2a00030c  43ffd59f  02c012f7
[    4.036154]
[    4.037630] ---[ end trace 0000000000000000 ]---

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 51 +++++++++++++++++++---------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 2f7f00f1bedb..8971414a2a60 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -292,8 +292,39 @@ enum chip_id {
 struct it66121_chip_info {
 	enum chip_id id;
 	u16 vid, pid;
+	char name[24];
 };
 
+static const struct it66121_chip_info it66121_chip_info[] = {
+	{
+		.id = ID_IT66121,
+		.vid = 0x4954,
+		.pid = 0x0612,
+		.name = "IT66121",
+	},
+	{
+		.id = ID_IT6610,
+		.vid = 0xca00,
+		.pid = 0x0611,
+		.name = "IT6610",
+	},
+};
+
+static const struct it66121_chip_info *
+it66121_get_match_data(u16 vender_id, u16 device_id)
+{
+	const struct it66121_chip_info *info;
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(it66121_chip_info); i++) {
+		info = &it66121_chip_info[i];
+		if (info->vid == vender_id && info->pid == device_id)
+			return info;
+	}
+
+	return NULL;
+}
+
 struct it66121_ctx {
 	struct regmap *regmap;
 	struct drm_bridge bridge;
@@ -1677,28 +1708,16 @@ static void it66121_remove(struct i2c_client *client)
 	mutex_destroy(&ctx->lock);
 }
 
-static const struct it66121_chip_info it66121_chip_info = {
-	.id = ID_IT66121,
-	.vid = 0x4954,
-	.pid = 0x0612,
-};
-
-static const struct it66121_chip_info it6610_chip_info = {
-	.id = ID_IT6610,
-	.vid = 0xca00,
-	.pid = 0x0611,
-};
-
 static const struct of_device_id it66121_dt_match[] = {
-	{ .compatible = "ite,it66121", &it66121_chip_info },
-	{ .compatible = "ite,it6610", &it6610_chip_info },
+	{ .compatible = "ite,it66121", &it66121_chip_info[0] },
+	{ .compatible = "ite,it6610", &it66121_chip_info[1] },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, it66121_dt_match);
 
 static const struct i2c_device_id it66121_id[] = {
-	{ "it66121", (kernel_ulong_t) &it66121_chip_info },
-	{ "it6610", (kernel_ulong_t) &it6610_chip_info },
+	{ "it66121", (kernel_ulong_t) &it66121_chip_info[0] },
+	{ "it6610", (kernel_ulong_t) &it66121_chip_info[1] },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, it66121_id);
-- 
2.34.1

Re: [PATCH 7/8] drm/bridge: it66121: Add another implementation for getting match data

[Dmitry Baryshkov]

On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> Because if use the i2c_get_match_data() function on our platform, the
> following error will emerge.
>
> [    3.597872] ACPI: bus type drm_connector registered
> [    3.602877] loongson 0000:00:06.1: Found LS7A1000 bridge chipset, revision: 1
> [    3.610013] loongson 0000:00:06.1: [drm] dc: 264MHz, gmc: 529MHz, gpu: 529MHz
> [    3.617111] loongson 0000:00:06.1: [drm] Dedicated vram start: 0xe0030000000, size: 64MiB
> [    3.633628] loongson 0000:00:06.1: [drm] Loongson VBIOS version: 0.3
> [    3.639944] loongson 0000:00:06.1: [drm] Loongson VBIOS: has 8 DCBs
> [    3.646271] Console: switching to colour dummy device 80x25
> [    3.651902] loongson 0000:00:06.1: [drm] VRAM: 4096 pages ready
> [    3.657791] loongson 0000:00:06.1: [drm] GTT: 32768 pages ready
> [    3.663731] loongson 0000:00:06.1: [drm] lsdc-i2c0(sda pin mask=1, scl pin mask=2) created
> [    3.671966] loongson 0000:00:06.1: [drm] lsdc-i2c1(sda pin mask=4, scl pin mask=8) created
> [    3.680188] loongson 0000:00:06.1: [drm] DisplayPipe-0 has DVO-0 connector
> [    3.711860] debugfs: Directory '1-004c' with parent 'regmap' already present!
> [    3.720312] CPU 0 Unable to handle kernel paging request at virtual address 0000000000000090, era == 9000000002faffec, ra == 9000000002faffc8
> [    3.732935] Oops[#1]:
> [    3.735187] CPU: 0 PID: 8 Comm: kworker/0:0 Not tainted 6.6.0-rc3+ #941
> [    3.741757] Hardware name: Loongson Loongson-3A5000-7A1000-1w-ML5A/Loongson-LS3A5000-7A1000-1w-ML5A, BIOS vUDK2018-LoongArch-V4.0.05-test-stable202305 06/
> [    3.755500] Workqueue: events work_for_cpu_fn
> [    3.759833] pc 9000000002faffec ra 9000000002faffc8 tp 900000010017c000 sp 900000010017fb80
> [    3.768132] a0 0000000000000000 a1 9000000101c00420 a2 0000000000000001 a3 900000010017fbc0
> [    3.776430] a4 0000000000000004 a5 0000000000000000 a6 0000000000000001 a7 0000000000000001
> [    3.784727] t0 0000000000000000 t1 0000000000000001 t2 0000000006124954 t3 0000000000000012
> [    3.793026] t4 0000000000004954 t5 900000000800b530 t6 0000000000000000 t7 0000000000018500
> [    3.801324] t8 90000001019c90c0 u0 9000000101c00c00 s9 fffffffffffff000 s0 9000000101c00400
> [    3.809621] s1 0000000000000000 s2 0000000000000000 s3 9000000003c42cb0 s4 9000000101c00420
> [    3.817920] s5 900000010017fc48 s6 0000000000000000 s7 0000000000000000 s8 90000000037fa000
> [    3.826219]    ra: 9000000002faffc8 i2c_get_match_data+0x24/0xac
> [    3.832189]   ERA: 9000000002faffec i2c_get_match_data+0x48/0xac
> [    3.838156]  CRMD: 000000b0 (PLV0 -IE -DA +PG DACF=CC DACM=CC -WE)
> [    3.844303]  PRMD: 00000004 (PPLV0 +PIE -PWE)
> [    3.848631]  EUEN: 00000000 (-FPE -SXE -ASXE -BTE)
> [    3.853391]  ECFG: 00071c1d (LIE=0,2-4,10-12 VS=7)
> [    3.858149] ESTAT: 00010000 [PIL] (IS= ECode=1 EsubCode=0)
> [    3.863599]  BADV: 0000000000000090
> [    3.867058]  PRID: 0014c011 (Loongson-64bit, Loongson-3A5000)
> [    3.872764] Modules linked in:
> [    3.875793] Process kworker/0:0 (pid: 8, threadinfo=(____ptrval____), task=(____ptrval____))
> [    3.884180] Stack : 9000000101c00400 0000000000000000 900000010177f428 9000000002ceb734
> [    3.892137]         9000000101c00404 90000000037fa000 900000010017fc60 9000000101c00400
> [    3.900093]         9000000016124954 8c326e68047d36df 0000004c037fa000 900000010017fc50
> [    3.908050]         0000000000000080 9000000003c44490 900000010017fcb0 90000001003e9ec0
> [    3.916007]         0000000000000001 900000010044f000 900000010017fc60 9000000101c00400
> [    3.923964]         90000000037fa000 9000000002cf05a8 90000001003e8000 900000010017fcb2
> [    3.931920]         000000304c17fc68 0000000000000000 000000000000004c 0000000000000000
> [    3.939877]         4031323136365449 0000000000006334 004c000000000000 0000000000000000
> [    3.947833]         0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [    3.955790]         0000000000000000 0000000000000000 9000000100449a98 8c326e68047d36df
> [    3.963746]         ...
> [    3.966170] Call Trace:
> [    3.966172] [<9000000002faffec>] i2c_get_match_data+0x48/0xac
> [    3.974304] [<9000000002ceb734>] it66121_create_bridge+0x15c/0x404

This function gets added only in patch 8, it is not broken otherwise.

I can only repeat my suggestion from the previous series: register a
proper I2C device. Don't try to overplay Linux device model.

> [    3.980446] [<9000000002cf05a8>] ls7a1000_output_init+0x228/0x248
> [    3.986503] [<9000000002cee494>] lsdc_pci_probe+0x4dc/0x5b0
> [    3.992038] [<9000000002b86b50>] local_pci_probe+0x48/0xa0
> [    3.997487] [<90000000023b5d04>] work_for_cpu_fn+0x1c/0x30
> [    4.002936] [<90000000023b7c0c>] process_one_work+0x14c/0x288
> [    4.008644] [<90000000023b8084>] worker_thread+0x33c/0x438
> [    4.014092] [<90000000023c1ccc>] kthread+0x124/0x130
> [    4.019023] [<9000000002381188>] ret_from_kernel_thread+0xc/0xa4
> [    4.024991]
> [    4.026463] Code: 26000079  02c08063  4c000020 <28c24318> 43ffe31f  43ffdeff  2a00030c  43ffd59f  02c012f7
> [    4.036154]
> [    4.037630] ---[ end trace 0000000000000000 ]---
>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 51 +++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 2f7f00f1bedb..8971414a2a60 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -292,8 +292,39 @@ enum chip_id {
>  struct it66121_chip_info {
>         enum chip_id id;
>         u16 vid, pid;
> +       char name[24];
>  };
>
> +static const struct it66121_chip_info it66121_chip_info[] = {
> +       {
> +               .id = ID_IT66121,
> +               .vid = 0x4954,
> +               .pid = 0x0612,
> +               .name = "IT66121",
> +       },
> +       {
> +               .id = ID_IT6610,
> +               .vid = 0xca00,
> +               .pid = 0x0611,
> +               .name = "IT6610",
> +       },
> +};
> +
> +static const struct it66121_chip_info *
> +it66121_get_match_data(u16 vender_id, u16 device_id)
> +{
> +       const struct it66121_chip_info *info;
> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(it66121_chip_info); i++) {
> +               info = &it66121_chip_info[i];
> +               if (info->vid == vender_id && info->pid == device_id)
> +                       return info;
> +       }
> +
> +       return NULL;
> +}
> +
>  struct it66121_ctx {
>         struct regmap *regmap;
>         struct drm_bridge bridge;
> @@ -1677,28 +1708,16 @@ static void it66121_remove(struct i2c_client *client)
>         mutex_destroy(&ctx->lock);
>  }
>
> -static const struct it66121_chip_info it66121_chip_info = {
> -       .id = ID_IT66121,
> -       .vid = 0x4954,
> -       .pid = 0x0612,
> -};
> -
> -static const struct it66121_chip_info it6610_chip_info = {
> -       .id = ID_IT6610,
> -       .vid = 0xca00,
> -       .pid = 0x0611,
> -};
> -
>  static const struct of_device_id it66121_dt_match[] = {
> -       { .compatible = "ite,it66121", &it66121_chip_info },
> -       { .compatible = "ite,it6610", &it6610_chip_info },
> +       { .compatible = "ite,it66121", &it66121_chip_info[0] },
> +       { .compatible = "ite,it6610", &it66121_chip_info[1] },
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, it66121_dt_match);
>
>  static const struct i2c_device_id it66121_id[] = {
> -       { "it66121", (kernel_ulong_t) &it66121_chip_info },
> -       { "it6610", (kernel_ulong_t) &it6610_chip_info },
> +       { "it66121", (kernel_ulong_t) &it66121_chip_info[0] },
> +       { "it6610", (kernel_ulong_t) &it66121_chip_info[1] },
>         { }
>  };
>  MODULE_DEVICE_TABLE(i2c, it66121_id);
> --
> 2.34.1
>


-- 
With best wishes
Dmitry

[PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

The it66121_create_bridge() and it66121_destroy_bridge() are added to
export the core functionalities. Create a connector manually by using
bridge connector helpers when link as a lib.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 134 +++++++++++++++++++--------
 include/drm/bridge/ite-it66121.h     |  17 ++++
 2 files changed, 113 insertions(+), 38 deletions(-)
 create mode 100644 include/drm/bridge/ite-it66121.h

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 8971414a2a60..f5968b679c5d 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -22,6 +22,7 @@
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_modes.h>
 #include <drm/drm_print.h>
@@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
 				 enum drm_bridge_attach_flags flags)
 {
 	struct it66121_ctx *ctx = bridge_to_it66121(bridge);
+	struct drm_bridge *next_bridge = ctx->next_bridge;
+	struct drm_encoder *encoder = bridge->encoder;
 	int ret;
 
-	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
-		return -EINVAL;
+	if (next_bridge) {
+		if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
+			WARN_ON(1);
+			flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
+		}
+		ret = drm_bridge_attach(encoder, next_bridge, bridge, flags);
+		if (ret)
+			return ret;
+	} else {
+		struct drm_connector *connector;
 
-	ret = drm_bridge_attach(bridge->encoder, ctx->next_bridge, bridge, flags);
-	if (ret)
-		return ret;
+		if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
+			WARN_ON(1);
+
+		connector = drm_bridge_connector_init(bridge->dev, encoder);
+		if (IS_ERR(connector))
+			return PTR_ERR(connector);
+
+		drm_connector_attach_encoder(connector, encoder);
+
+		ctx->connector = connector;
+	}
 
 	if (ctx->info->id == ID_IT66121) {
 		ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
@@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
 	"vcn33", "vcn18", "vrf12"
 };
 
-static int it66121_probe(struct i2c_client *client)
+int it66121_create_bridge(struct i2c_client *client, bool of_support,
+			  bool hpd_support, bool audio_support,
+			  struct drm_bridge **bridge)
 {
+	struct device *dev = &client->dev;
 	int ret;
 	struct it66121_ctx *ctx;
-	struct device *dev = &client->dev;
-
-	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
-		dev_err(dev, "I2C check functionality failed.\n");
-		return -ENXIO;
-	}
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
@@ -1649,24 +1665,19 @@ static int it66121_probe(struct i2c_client *client)
 
 	ctx->dev = dev;
 	ctx->client = client;
-	ctx->info = i2c_get_match_data(client);
-
-	ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
-	if (ret)
-		return ret;
-
-	ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
-	if (ret)
-		return ret;
-
-	i2c_set_clientdata(client, ctx);
 	mutex_init(&ctx->lock);
 
-	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it66121_supplies),
-					     it66121_supplies);
-	if (ret) {
-		dev_err(dev, "Failed to enable power supplies\n");
-		return ret;
+	if (of_support) {
+		ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
+		if (ret)
+			return ret;
+
+		ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
+		if (ret)
+			return ret;
+	} else {
+		ctx->bus_width = 24;
+		ctx->next_bridge = NULL;
 	}
 
 	it66121_hw_reset(ctx);
@@ -1679,33 +1690,80 @@ static int it66121_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	if (ctx->vender_id != ctx->info->vid ||
-	    ctx->device_id != ctx->info->pid)
+	ctx->info = it66121_get_match_data(ctx->vender_id, ctx->device_id);
+	if (!ctx->info)
 		return -ENODEV;
 
-	ret = devm_request_threaded_irq(dev, client->irq, NULL,	it66121_irq_threaded_handler,
-					IRQF_ONESHOT, dev_name(dev), ctx);
-	if (ret < 0) {
-		dev_err(dev, "Failed to request irq %d:%d\n", client->irq, ret);
-		return ret;
+	if (hpd_support) {
+		ret = devm_request_threaded_irq(dev, client->irq, NULL,
+						it66121_irq_threaded_handler,
+						IRQF_ONESHOT, dev_name(dev),
+						ctx);
+		if (ret < 0) {
+			dev_err(dev, "Failed to request irq: %d\n", ret);
+			return ret;
+		}
 	}
 
 	it66121_bridge_init_base(&ctx->bridge, dev->of_node, true);
 
-	it66121_audio_codec_init(ctx, dev);
+	if (audio_support)
+		it66121_audio_codec_init(ctx, dev);
+
+	*bridge = &ctx->bridge;
 
 	dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
 		 ctx->vender_id, ctx->device_id, ctx->revision);
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(it66121_create_bridge);
+
+static int it66121_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct it66121_ctx *ctx;
+	struct drm_bridge *bridge;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		dev_err(dev, "I2C check functionality failed.\n");
+		return -ENXIO;
+	}
+
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it66121_supplies),
+					     it66121_supplies);
+	if (ret) {
+		dev_err(dev, "Failed to enable power supplies\n");
+		return ret;
+	}
+
+	ret = it66121_create_bridge(client, true, true, true, &bridge);
+	if (ret)
+		return ret;
+
+	ctx = bridge_to_it66121(bridge);
+
+	i2c_set_clientdata(client, ctx);
+
+	return 0;
+}
+
+void it66121_destroy_bridge(struct drm_bridge *bridge)
+{
+	struct it66121_ctx *ctx = bridge_to_it66121(bridge);
+
+	drm_bridge_remove(bridge);
+
+	mutex_destroy(&ctx->lock);
+}
+EXPORT_SYMBOL_GPL(it66121_destroy_bridge);
 
 static void it66121_remove(struct i2c_client *client)
 {
 	struct it66121_ctx *ctx = i2c_get_clientdata(client);
 
-	drm_bridge_remove(&ctx->bridge);
-	mutex_destroy(&ctx->lock);
+	it66121_destroy_bridge(&ctx->bridge);
 }
 
 static const struct of_device_id it66121_dt_match[] = {
diff --git a/include/drm/bridge/ite-it66121.h b/include/drm/bridge/ite-it66121.h
new file mode 100644
index 000000000000..e6753f695b7f
--- /dev/null
+++ b/include/drm/bridge/ite-it66121.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __ITE_IT66121_H__
+#define __ITE_IT66121_H__
+
+#include <linux/i2c.h>
+
+#include <drm/drm_bridge.h>
+#include <drm/drm_device.h>
+
+int it66121_create_bridge(struct i2c_client *client, bool of_support,
+			  bool hpd_support, bool audio_support,
+			  struct drm_bridge **bridge);
+
+void it66121_destroy_bridge(struct drm_bridge *bridge);
+
+#endif
-- 
2.34.1

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Dmitry Baryshkov]

On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> From: Sui Jingfeng <suijingfeng@loongson.cn>
>
> The it66121_create_bridge() and it66121_destroy_bridge() are added to
> export the core functionalities. Create a connector manually by using
> bridge connector helpers when link as a lib.
>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 134 +++++++++++++++++++--------
>  include/drm/bridge/ite-it66121.h     |  17 ++++
>  2 files changed, 113 insertions(+), 38 deletions(-)
>  create mode 100644 include/drm/bridge/ite-it66121.h
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 8971414a2a60..f5968b679c5d 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -22,6 +22,7 @@
>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_modes.h>
>  #include <drm/drm_print.h>
> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
>                                  enum drm_bridge_attach_flags flags)
>  {
>         struct it66121_ctx *ctx = bridge_to_it66121(bridge);
> +       struct drm_bridge *next_bridge = ctx->next_bridge;
> +       struct drm_encoder *encoder = bridge->encoder;
>         int ret;
>
> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> -               return -EINVAL;
> +       if (next_bridge) {
> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> +                       WARN_ON(1);

Why? At least use WARN() instead

> +                       flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
> +               }
> +               ret = drm_bridge_attach(encoder, next_bridge, bridge, flags);
> +               if (ret)
> +                       return ret;
> +       } else {
> +               struct drm_connector *connector;
>
> -       ret = drm_bridge_attach(bridge->encoder, ctx->next_bridge, bridge, flags);
> -       if (ret)
> -               return ret;
> +               if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> +                       WARN_ON(1);

No. It is perfectly fine to create attach a bridge with no next_bridge
and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.

> +
> +               connector = drm_bridge_connector_init(bridge->dev, encoder);
> +               if (IS_ERR(connector))
> +                       return PTR_ERR(connector);
> +
> +               drm_connector_attach_encoder(connector, encoder);

This goes into your device driver.

> +
> +               ctx->connector = connector;
> +       }
>
>         if (ctx->info->id == ID_IT66121) {
>                 ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
> @@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
>         "vcn33", "vcn18", "vrf12"
>  };
>
> -static int it66121_probe(struct i2c_client *client)
> +int it66121_create_bridge(struct i2c_client *client, bool of_support,
> +                         bool hpd_support, bool audio_support,
> +                         struct drm_bridge **bridge)
>  {
> +       struct device *dev = &client->dev;
>         int ret;
>         struct it66121_ctx *ctx;
> -       struct device *dev = &client->dev;
> -
> -       if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> -               dev_err(dev, "I2C check functionality failed.\n");
> -               return -ENXIO;
> -       }
>
>         ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>         if (!ctx)
> @@ -1649,24 +1665,19 @@ static int it66121_probe(struct i2c_client *client)
>
>         ctx->dev = dev;
>         ctx->client = client;
> -       ctx->info = i2c_get_match_data(client);
> -
> -       ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
> -       if (ret)
> -               return ret;
> -
> -       ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
> -       if (ret)
> -               return ret;
> -
> -       i2c_set_clientdata(client, ctx);
>         mutex_init(&ctx->lock);
>
> -       ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it66121_supplies),
> -                                            it66121_supplies);
> -       if (ret) {
> -               dev_err(dev, "Failed to enable power supplies\n");
> -               return ret;
> +       if (of_support) {
> +               ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
> +               if (ret)
> +                       return ret;
> +
> +               ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
> +               if (ret)
> +                       return ret;
> +       } else {
> +               ctx->bus_width = 24;
> +               ctx->next_bridge = NULL;
>         }

A better alternative would be to turn OF calls into fwnode calls and
to populate the fwnode properties. See
drivers/platform/x86/intel/chtwc_int33fe.c for example.

>
>         it66121_hw_reset(ctx);
> @@ -1679,33 +1690,80 @@ static int it66121_probe(struct i2c_client *client)
>         if (ret)
>                 return ret;
>
> -       if (ctx->vender_id != ctx->info->vid ||
> -           ctx->device_id != ctx->info->pid)
> +       ctx->info = it66121_get_match_data(ctx->vender_id, ctx->device_id);
> +       if (!ctx->info)
>                 return -ENODEV;
>
> -       ret = devm_request_threaded_irq(dev, client->irq, NULL, it66121_irq_threaded_handler,
> -                                       IRQF_ONESHOT, dev_name(dev), ctx);
> -       if (ret < 0) {
> -               dev_err(dev, "Failed to request irq %d:%d\n", client->irq, ret);
> -               return ret;
> +       if (hpd_support) {

Who handles HPD in your platform case?

> +               ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +                                               it66121_irq_threaded_handler,
> +                                               IRQF_ONESHOT, dev_name(dev),
> +                                               ctx);
> +               if (ret < 0) {
> +                       dev_err(dev, "Failed to request irq: %d\n", ret);
> +                       return ret;
> +               }
>         }
>
>         it66121_bridge_init_base(&ctx->bridge, dev->of_node, true);
>
> -       it66121_audio_codec_init(ctx, dev);
> +       if (audio_support)
> +               it66121_audio_codec_init(ctx, dev);
> +
> +       *bridge = &ctx->bridge;
>
>         dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
>                  ctx->vender_id, ctx->device_id, ctx->revision);
>
>         return 0;
>  }
> +EXPORT_SYMBOL_GPL(it66121_create_bridge);
> +
> +static int it66121_probe(struct i2c_client *client)
> +{
> +       struct device *dev = &client->dev;
> +       struct it66121_ctx *ctx;
> +       struct drm_bridge *bridge;
> +       int ret;
> +
> +       if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +               dev_err(dev, "I2C check functionality failed.\n");
> +               return -ENXIO;
> +       }
> +
> +       ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it66121_supplies),
> +                                            it66121_supplies);
> +       if (ret) {
> +               dev_err(dev, "Failed to enable power supplies\n");
> +               return ret;
> +       }
> +
> +       ret = it66121_create_bridge(client, true, true, true, &bridge);
> +       if (ret)
> +               return ret;
> +
> +       ctx = bridge_to_it66121(bridge);
> +
> +       i2c_set_clientdata(client, ctx);
> +
> +       return 0;
> +}
> +
> +void it66121_destroy_bridge(struct drm_bridge *bridge)
> +{
> +       struct it66121_ctx *ctx = bridge_to_it66121(bridge);
> +
> +       drm_bridge_remove(bridge);
> +
> +       mutex_destroy(&ctx->lock);
> +}
> +EXPORT_SYMBOL_GPL(it66121_destroy_bridge);
>
>  static void it66121_remove(struct i2c_client *client)
>  {
>         struct it66121_ctx *ctx = i2c_get_clientdata(client);
>
> -       drm_bridge_remove(&ctx->bridge);
> -       mutex_destroy(&ctx->lock);
> +       it66121_destroy_bridge(&ctx->bridge);
>  }
>
>  static const struct of_device_id it66121_dt_match[] = {
> diff --git a/include/drm/bridge/ite-it66121.h b/include/drm/bridge/ite-it66121.h
> new file mode 100644
> index 000000000000..e6753f695b7f
> --- /dev/null
> +++ b/include/drm/bridge/ite-it66121.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ITE_IT66121_H__
> +#define __ITE_IT66121_H__
> +
> +#include <linux/i2c.h>
> +
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_device.h>
> +
> +int it66121_create_bridge(struct i2c_client *client, bool of_support,
> +                         bool hpd_support, bool audio_support,
> +                         struct drm_bridge **bridge);
> +
> +void it66121_destroy_bridge(struct drm_bridge *bridge);
> +
> +#endif
> --
> 2.34.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,

Thanks a lot for reviewing!


On 2023/11/15 00:30, Dmitry Baryshkov wrote:
> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> The it66121_create_bridge() and it66121_destroy_bridge() are added to
>> export the core functionalities. Create a connector manually by using
>> bridge connector helpers when link as a lib.
>>
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/bridge/ite-it66121.c | 134 +++++++++++++++++++--------
>>   include/drm/bridge/ite-it66121.h     |  17 ++++
>>   2 files changed, 113 insertions(+), 38 deletions(-)
>>   create mode 100644 include/drm/bridge/ite-it66121.h
>>
>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
>> index 8971414a2a60..f5968b679c5d 100644
>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
>> @@ -22,6 +22,7 @@
>>
>>   #include <drm/drm_atomic_helper.h>
>>   #include <drm/drm_bridge.h>
>> +#include <drm/drm_bridge_connector.h>
>>   #include <drm/drm_edid.h>
>>   #include <drm/drm_modes.h>
>>   #include <drm/drm_print.h>
>> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
>>                                   enum drm_bridge_attach_flags flags)
>>   {
>>          struct it66121_ctx *ctx = bridge_to_it66121(bridge);
>> +       struct drm_bridge *next_bridge = ctx->next_bridge;
>> +       struct drm_encoder *encoder = bridge->encoder;
>>          int ret;
>>
>> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>> -               return -EINVAL;
>> +       if (next_bridge) {
>> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
>> +                       WARN_ON(1);
> Why? At least use WARN() instead

Originally I want to




>> +                       flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
>> +               }
>> +               ret = drm_bridge_attach(encoder, next_bridge, bridge, flags);
>> +               if (ret)
>> +                       return ret;
>> +       } else {
>> +               struct drm_connector *connector;
>>
>> -       ret = drm_bridge_attach(bridge->encoder, ctx->next_bridge, bridge, flags);
>> -       if (ret)
>> -               return ret;
>> +               if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
>> +                       WARN_ON(1);
> No. It is perfectly fine to create attach a bridge with no next_bridge
> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
>

The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
the bridge shall not create a drm_connector. So I think if a display
bridge driver don't have a next bridge attached (Currently, this is
told by the DT), it says that this is a non-DT environment. On such
a case, this display bridge driver(it66121.ko) should behavior like
a *agent*. Because the upstream port of it66121 is the DVO port of
the display controller, the downstream port of it66121 is the HDMI
connector. it66121 is on the middle. So I think the it66121.ko should
handle all of troubles on behalf of the display controller drivers.

Therefore (when in non-DT use case), the display controller drivers
side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
Which is to hint that the it66121 should totally in charge of those
tasks (either by using bridge connector helper or create a connector
manually). I don't understand on such a case, why bother display
controller drivers anymore.


>> +
>> +               connector = drm_bridge_connector_init(bridge->dev, encoder);
>> +               if (IS_ERR(connector))
>> +                       return PTR_ERR(connector);
>> +
>> +               drm_connector_attach_encoder(connector, encoder);
> This goes into your device driver.
>
>> +
>> +               ctx->connector = connector;
>> +       }
>>
>>          if (ctx->info->id == ID_IT66121) {
>>                  ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
>> @@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
>>          "vcn33", "vcn18", "vrf12"
>>   };
>

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Dmitry Baryshkov]

On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Hi,
>
> Thanks a lot for reviewing!
>
>
> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
> > On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> >> From: Sui Jingfeng <suijingfeng@loongson.cn>
> >>
> >> The it66121_create_bridge() and it66121_destroy_bridge() are added to
> >> export the core functionalities. Create a connector manually by using
> >> bridge connector helpers when link as a lib.
> >>
> >> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> >> ---
> >>   drivers/gpu/drm/bridge/ite-it66121.c | 134 +++++++++++++++++++--------
> >>   include/drm/bridge/ite-it66121.h     |  17 ++++
> >>   2 files changed, 113 insertions(+), 38 deletions(-)
> >>   create mode 100644 include/drm/bridge/ite-it66121.h
> >>
> >> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> >> index 8971414a2a60..f5968b679c5d 100644
> >> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> >> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> >> @@ -22,6 +22,7 @@
> >>
> >>   #include <drm/drm_atomic_helper.h>
> >>   #include <drm/drm_bridge.h>
> >> +#include <drm/drm_bridge_connector.h>
> >>   #include <drm/drm_edid.h>
> >>   #include <drm/drm_modes.h>
> >>   #include <drm/drm_print.h>
> >> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
> >>                                   enum drm_bridge_attach_flags flags)
> >>   {
> >>          struct it66121_ctx *ctx = bridge_to_it66121(bridge);
> >> +       struct drm_bridge *next_bridge = ctx->next_bridge;
> >> +       struct drm_encoder *encoder = bridge->encoder;
> >>          int ret;
> >>
> >> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> >> -               return -EINVAL;
> >> +       if (next_bridge) {
> >> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> >> +                       WARN_ON(1);
> > Why? At least use WARN() instead
>
> Originally I want to
>
>
>
>
> >> +                       flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
> >> +               }
> >> +               ret = drm_bridge_attach(encoder, next_bridge, bridge, flags);
> >> +               if (ret)
> >> +                       return ret;
> >> +       } else {
> >> +               struct drm_connector *connector;
> >>
> >> -       ret = drm_bridge_attach(bridge->encoder, ctx->next_bridge, bridge, flags);
> >> -       if (ret)
> >> -               return ret;
> >> +               if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> >> +                       WARN_ON(1);
> > No. It is perfectly fine to create attach a bridge with no next_bridge
> > and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
> >
>
> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
> the bridge shall not create a drm_connector. So I think if a display
> bridge driver don't have a next bridge attached (Currently, this is
> told by the DT), it says that this is a non-DT environment. On such
> a case, this display bridge driver(it66121.ko) should behavior like
> a *agent*. Because the upstream port of it66121 is the DVO port of
> the display controller, the downstream port of it66121 is the HDMI
> connector. it66121 is on the middle. So I think the it66121.ko should
> handle all of troubles on behalf of the display controller drivers.

No. Don't make decisions for the other drivers. They might have different needs.

> Therefore (when in non-DT use case), the display controller drivers
> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
> Which is to hint that the it66121 should totally in charge of those
> tasks (either by using bridge connector helper or create a connector
> manually). I don't understand on such a case, why bother display
> controller drivers anymore.

This is the reason why we had introduced this flag. It allows the
driver to customise the connector. It even allows the driver to
implement a connector on its own, completely ignoring the
drm_bridge_connector.

>
>
> >> +
> >> +               connector = drm_bridge_connector_init(bridge->dev, encoder);
> >> +               if (IS_ERR(connector))
> >> +                       return PTR_ERR(connector);
> >> +
> >> +               drm_connector_attach_encoder(connector, encoder);
> > This goes into your device driver.
> >
> >> +
> >> +               ctx->connector = connector;
> >> +       }
> >>
> >>          if (ctx->info->id == ID_IT66121) {
> >>                  ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
> >> @@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
> >>          "vcn33", "vcn18", "vrf12"
> >>   };
> >



-- 
With best wishes
Dmitry

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,


On 2023/11/16 17:30, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>> Hi,
>>
>> Thanks a lot for reviewing!
>>
>>
>> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
>>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>
>>>> The it66121_create_bridge() and it66121_destroy_bridge() are added to
>>>> export the core functionalities. Create a connector manually by using
>>>> bridge connector helpers when link as a lib.
>>>>
>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> ---
>>>>    drivers/gpu/drm/bridge/ite-it66121.c | 134 +++++++++++++++++++--------
>>>>    include/drm/bridge/ite-it66121.h     |  17 ++++
>>>>    2 files changed, 113 insertions(+), 38 deletions(-)
>>>>    create mode 100644 include/drm/bridge/ite-it66121.h
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
>>>> index 8971414a2a60..f5968b679c5d 100644
>>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
>>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
>>>> @@ -22,6 +22,7 @@
>>>>
>>>>    #include <drm/drm_atomic_helper.h>
>>>>    #include <drm/drm_bridge.h>
>>>> +#include <drm/drm_bridge_connector.h>
>>>>    #include <drm/drm_edid.h>
>>>>    #include <drm/drm_modes.h>
>>>>    #include <drm/drm_print.h>
>>>> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
>>>>                                    enum drm_bridge_attach_flags flags)
>>>>    {
>>>>           struct it66121_ctx *ctx = bridge_to_it66121(bridge);
>>>> +       struct drm_bridge *next_bridge = ctx->next_bridge;
>>>> +       struct drm_encoder *encoder = bridge->encoder;
>>>>           int ret;
>>>>
>>>> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>>>> -               return -EINVAL;
>>>> +       if (next_bridge) {
>>>> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
>>>> +                       WARN_ON(1);
>>> Why? At least use WARN() instead
>> Originally I want to
>>
>>
>>
>>
>>>> +                       flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
>>>> +               }
>>>> +               ret = drm_bridge_attach(encoder, next_bridge, bridge, flags);
>>>> +               if (ret)
>>>> +                       return ret;
>>>> +       } else {
>>>> +               struct drm_connector *connector;
>>>>
>>>> -       ret = drm_bridge_attach(bridge->encoder, ctx->next_bridge, bridge, flags);
>>>> -       if (ret)
>>>> -               return ret;
>>>> +               if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
>>>> +                       WARN_ON(1);
>>> No. It is perfectly fine to create attach a bridge with no next_bridge
>>> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
>>>
>> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
>> the bridge shall not create a drm_connector. So I think if a display
>> bridge driver don't have a next bridge attached (Currently, this is
>> told by the DT), it says that this is a non-DT environment. On such
>> a case, this display bridge driver(it66121.ko) should behavior like
>> a *agent*. Because the upstream port of it66121 is the DVO port of
>> the display controller, the downstream port of it66121 is the HDMI
>> connector. it66121 is on the middle. So I think the it66121.ko should
>> handle all of troubles on behalf of the display controller drivers.
> No. Don't make decisions for the other drivers. They might have different needs.

[...]


>
>> Therefore (when in non-DT use case), the display controller drivers
>> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
>> Which is to hint that the it66121 should totally in charge of those
>> tasks (either by using bridge connector helper or create a connector
>> manually). I don't understand on such a case, why bother display
>> controller drivers anymore.
> This is the reason why we had introduced this flag. It allows the
> driver to customise the connector. It even allows the driver to
> implement a connector on its own, completely ignoring the
> drm_bridge_connector.


I know what you said is right in the sense of the universe cases,
but I think the most frequent(majority) use case is that there is
only one display bridge on the middle. Therefore, I don't want to
movethe connector things into device driver if there is only one display 
bridge(say it66121) in the middle. After all, there is no *direct 
physical connection* from the perspective of the hardware. I means that 
there is no hardware wires connectthe HDMI connector and the DVO port. So display controller drivers 
should not interact with anything related with the connector on a 
perfect abstract on the software side. Especially for such a simple use 
case. It probably make senses to make a  decision for themost frequently use case, please also note
that this patch didn't introduce any-restriction for the more advance
uses cases(multiple bridges in the middle).


>>
>>>> +
>>>> +               connector = drm_bridge_connector_init(bridge->dev, encoder);
>>>> +               if (IS_ERR(connector))
>>>> +                       return PTR_ERR(connector);
>>>> +
>>>> +               drm_connector_attach_encoder(connector, encoder);
>>> This goes into your device driver.
>>>
>>>> +
>>>> +               ctx->connector = connector;
>>>> +       }
>>>>
>>>>           if (ctx->info->id == ID_IT66121) {
>>>>                   ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
>>>> @@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
>>>>           "vcn33", "vcn18", "vrf12"
>>>>    };
>
>

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Dmitry Baryshkov]

On Thu, 16 Nov 2023 at 12:13, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Hi,
>
>
> On 2023/11/16 17:30, Dmitry Baryshkov wrote:
> > On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> >> Hi,
> >>
> >> Thanks a lot for reviewing!
> >>
> >>
> >> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
> >>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> >>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
> >>>>
> >>>> The it66121_create_bridge() and it66121_destroy_bridge() are added to
> >>>> export the core functionalities. Create a connector manually by using
> >>>> bridge connector helpers when link as a lib.
> >>>>
> >>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> >>>> ---
> >>>>    drivers/gpu/drm/bridge/ite-it66121.c | 134 +++++++++++++++++++--------
> >>>>    include/drm/bridge/ite-it66121.h     |  17 ++++
> >>>>    2 files changed, 113 insertions(+), 38 deletions(-)
> >>>>    create mode 100644 include/drm/bridge/ite-it66121.h
> >>>>
> >>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> >>>> index 8971414a2a60..f5968b679c5d 100644
> >>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> >>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> >>>> @@ -22,6 +22,7 @@
> >>>>
> >>>>    #include <drm/drm_atomic_helper.h>
> >>>>    #include <drm/drm_bridge.h>
> >>>> +#include <drm/drm_bridge_connector.h>
> >>>>    #include <drm/drm_edid.h>
> >>>>    #include <drm/drm_modes.h>
> >>>>    #include <drm/drm_print.h>
> >>>> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
> >>>>                                    enum drm_bridge_attach_flags flags)
> >>>>    {
> >>>>           struct it66121_ctx *ctx = bridge_to_it66121(bridge);
> >>>> +       struct drm_bridge *next_bridge = ctx->next_bridge;
> >>>> +       struct drm_encoder *encoder = bridge->encoder;
> >>>>           int ret;
> >>>>
> >>>> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> >>>> -               return -EINVAL;
> >>>> +       if (next_bridge) {
> >>>> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> >>>> +                       WARN_ON(1);
> >>> Why? At least use WARN() instead
> >> Originally I want to
> >>
> >>
> >>
> >>
> >>>> +                       flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
> >>>> +               }
> >>>> +               ret = drm_bridge_attach(encoder, next_bridge, bridge, flags);
> >>>> +               if (ret)
> >>>> +                       return ret;
> >>>> +       } else {
> >>>> +               struct drm_connector *connector;
> >>>>
> >>>> -       ret = drm_bridge_attach(bridge->encoder, ctx->next_bridge, bridge, flags);
> >>>> -       if (ret)
> >>>> -               return ret;
> >>>> +               if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> >>>> +                       WARN_ON(1);
> >>> No. It is perfectly fine to create attach a bridge with no next_bridge
> >>> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
> >>>
> >> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
> >> the bridge shall not create a drm_connector. So I think if a display
> >> bridge driver don't have a next bridge attached (Currently, this is
> >> told by the DT), it says that this is a non-DT environment. On such
> >> a case, this display bridge driver(it66121.ko) should behavior like
> >> a *agent*. Because the upstream port of it66121 is the DVO port of
> >> the display controller, the downstream port of it66121 is the HDMI
> >> connector. it66121 is on the middle. So I think the it66121.ko should
> >> handle all of troubles on behalf of the display controller drivers.
> > No. Don't make decisions for the other drivers. They might have different needs.
>
> [...]
>
>
> >
> >> Therefore (when in non-DT use case), the display controller drivers
> >> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
> >> Which is to hint that the it66121 should totally in charge of those
> >> tasks (either by using bridge connector helper or create a connector
> >> manually). I don't understand on such a case, why bother display
> >> controller drivers anymore.
> > This is the reason why we had introduced this flag. It allows the
> > driver to customise the connector. It even allows the driver to
> > implement a connector on its own, completely ignoring the
> > drm_bridge_connector.
>
>
> I know what you said is right in the sense of the universe cases,
> but I think the most frequent(majority) use case is that there is
> only one display bridge on the middle. Therefore, I don't want to
> movethe connector things into device driver if there is only one display
> bridge(say it66121) in the middle. After all, there is no *direct
> physical connection* from the perspective of the hardware. I means that
> there is no hardware wires connectthe HDMI connector and the DVO port. So display controller drivers
> should not interact with anything related with the connector on a
> perfect abstract on the software side. Especially for such a simple use
> case. It probably make senses to make a  decision for themost frequently use case, please also note
> that this patch didn't introduce any-restriction for the more advance
> uses cases(multiple bridges in the middle).

So, for the sake of not having the connector in the display driver,
you want to add boilerplate code basically to each and every bridge
driver. In the end, they should all behave in the same way.

Moreover, there is no way this implementation can work without a
warning if there are two bridges in a chain and the it66121 is the
second (the last) one. The host can not specify the
DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> >>>> +                       WARN_ON(1);
> >>> No. It is perfectly fine to create attach a bridge with no next_bridge
> >>> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
> >>>
> >> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
> >> the bridge shall not create a drm_connector. So I think if a display
> >> bridge driver don't have a next bridge attached (Currently, this is
> >> told by the DT), it says that this is a non-DT environment. On such
> >> a case, this display bridge driver(it66121.ko) should behavior like
> >> a *agent*. Because the upstream port of it66121 is the DVO port of
> >> the display controller, the downstream port of it66121 is the HDMI
> >> connector. it66121 is on the middle. So I think the it66121.ko should
> >> handle all of troubles on behalf of the display controller drivers.
> > No. Don't make decisions for the other drivers. They might have different needs.
>
> [...]
>
>
> >
> >> Therefore (when in non-DT use case), the display controller drivers
> >> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
> >> Which is to hint that the it66121 should totally in charge of those
> >> tasks (either by using bridge connector helper or create a connector
> >> manually). I don't understand on such a case, why bother display
> >> controller drivers anymore.
> > This is the reason why we had introduced this flag. It allows the
> > driver to customise the connector. It even allows the driver to
> > implement a connector on its own, completely ignoring the
> > drm_bridge_connector.
>
>
> I know what you said is right in the sense of the universe cases,
> but I think the most frequent(majority) use case is that there is
> only one display bridge on the middle. Therefore, I don't want to
> movethe connector things into device driver if there is only one display
> bridge(say it66121) in the middle. After all, there is no *direct
> physical connection* from the perspective of the hardware. I means that
> there is no hardware wires connectthe HDMI connector and the DVO port. So display controller drivers
> should not interact with anything related with the connector on a
> perfect abstract on the software side. Especially for such a simple use
> case. It probably make senses to make a  decision for themost frequently use case, please also note
> that this patch didn't introduce any-restriction for the more advance
> uses cases(multiple bridges in the middle).

So, for the sake of not having the connector in the display driver,
you want to add boilerplate code basically to each and every bridge
driver. In the end, they should all behave in the same way.

Moreover, there is no way this implementation can work without a
warning if there are two bridges in a chain and the it66121 is the
second (the last) one. The host can not specify the
DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, it will cause a warning here. And
it can not omit the flag (as otherwise the first bridge will create a
connector, without consulting the second bridge).

>
>
> >>
> >>>> +
> >>>> +               connector = drm_bridge_connector_init(bridge->dev, encoder);
> >>>> +               if (IS_ERR(connector))
> >>>> +                       return PTR_ERR(connector);
> >>>> +
> >>>> +               drm_connector_attach_encoder(connector, encoder);
> >>> This goes into your device driver.
> >>>
> >>>> +
> >>>> +               ctx->connector = connector;
> >>>> +       }
> >>>>
> >>>>           if (ctx->info->id == ID_IT66121) {
> >>>>                   ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
> >>>> @@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
> >>>>           "vcn33", "vcn18", "vrf12"
> >>>>    };
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,


On 2023/11/16 19:19, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 12:13, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>> Hi,
>>
>>
>> On 2023/11/16 17:30, Dmitry Baryshkov wrote:
>>> On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>>> Hi,
>>>>
>>>> Thanks a lot for reviewing!
>>>>
>>>>
>>>> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
>>>>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>>
>>>>>> The it66121_create_bridge() and it66121_destroy_bridge() are added to
>>>>>> export the core functionalities. Create a connector manually by using
>>>>>> bridge connector helpers when link as a lib.
>>>>>>
>>>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>> ---
>>>>>>     drivers/gpu/drm/bridge/ite-it66121.c | 134 +++++++++++++++++++--------
>>>>>>     include/drm/bridge/ite-it66121.h     |  17 ++++
>>>>>>     2 files changed, 113 insertions(+), 38 deletions(-)
>>>>>>     create mode 100644 include/drm/bridge/ite-it66121.h
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>> index 8971414a2a60..f5968b679c5d 100644
>>>>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>> @@ -22,6 +22,7 @@
>>>>>>
>>>>>>     #include <drm/drm_atomic_helper.h>
>>>>>>     #include <drm/drm_bridge.h>
>>>>>> +#include <drm/drm_bridge_connector.h>
>>>>>>     #include <drm/drm_edid.h>
>>>>>>     #include <drm/drm_modes.h>
>>>>>>     #include <drm/drm_print.h>
>>>>>> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
>>>>>>                                     enum drm_bridge_attach_flags flags)
>>>>>>     {
>>>>>>            struct it66121_ctx *ctx = bridge_to_it66121(bridge);
>>>>>> +       struct drm_bridge *next_bridge = ctx->next_bridge;
>>>>>> +       struct drm_encoder *encoder = bridge->encoder;
>>>>>>            int ret;
>>>>>>
>>>>>> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>>>>>> -               return -EINVAL;
>>>>>> +       if (next_bridge) {
>>>>>> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
>>>>>> +                       WARN_ON(1);
>>>>> Why? At least use WARN() instead
>>>> Originally I want to
>>>>
>>>>
>>>>
>>>>
>>>>>> +                       flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
>>>>>> +               }
>>>>>> +               ret = drm_bridge_attach(encoder, next_bridge, bridge, flags);
>>>>>> +               if (ret)
>>>>>> +                       return ret;
>>>>>> +       } else {
>>>>>> +               struct drm_connector *connector;
>>>>>>
>>>>>> -       ret = drm_bridge_attach(bridge->encoder, ctx->next_bridge, bridge, flags);
>>>>>> -       if (ret)
>>>>>> -               return ret;
>>>>>> +               if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
>>>>>> +                       WARN_ON(1);
>>>>> No. It is perfectly fine to create attach a bridge with no next_bridge
>>>>> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
>>>>>
>>>> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
>>>> the bridge shall not create a drm_connector. So I think if a display
>>>> bridge driver don't have a next bridge attached (Currently, this is
>>>> told by the DT), it says that this is a non-DT environment. On such
>>>> a case, this display bridge driver(it66121.ko) should behavior like
>>>> a *agent*. Because the upstream port of it66121 is the DVO port of
>>>> the display controller, the downstream port of it66121 is the HDMI
>>>> connector. it66121 is on the middle. So I think the it66121.ko should
>>>> handle all of troubles on behalf of the display controller drivers.
>>> No. Don't make decisions for the other drivers. They might have different needs.
>> [...]
>>
>>
>>>> Therefore (when in non-DT use case), the display controller drivers
>>>> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
>>>> Which is to hint that the it66121 should totally in charge of those
>>>> tasks (either by using bridge connector helper or create a connector
>>>> manually). I don't understand on such a case, why bother display
>>>> controller drivers anymore.
>>> This is the reason why we had introduced this flag. It allows the
>>> driver to customise the connector. It even allows the driver to
>>> implement a connector on its own, completely ignoring the
>>> drm_bridge_connector.
>>
>> I know what you said is right in the sense of the universe cases,
>> but I think the most frequent(majority) use case is that there is
>> only one display bridge on the middle. Therefore, I don't want to
>> movethe connector things into device driver if there is only one display
>> bridge(say it66121) in the middle. After all, there is no *direct
>> physical connection* from the perspective of the hardware. I means that
>> there is no hardware wires connectthe HDMI connector and the DVO port. So display controller drivers
>> should not interact with anything related with the connector on a
>> perfect abstract on the software side. Especially for such a simple use
>> case. It probably make senses to make a  decision for themost frequently use case, please also note
>> that this patch didn't introduce any-restriction for the more advance
>> uses cases(multiple bridges in the middle).
> So, for the sake of not having the connector in the display driver,
> you want to add boilerplate code basically to each and every bridge
> driver. In the end, they should all behave in the same way.
>
> Moreover, there is no way this implementation can work without a
> warning if there are two bridges in a chain and the it66121 is the
> second (the last) one. The host can not specify the
> DRM_BRIDGE_ATTACH_NO_CONNECTOR)
>>>>>> +                       WARN_ON(1);
>>>>> No. It is perfectly fine to create attach a bridge with no next_bridge
>>>>> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
>>>>>
>>>> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
>>>> the bridge shall not create a drm_connector. So I think if a display
>>>> bridge driver don't have a next bridge attached (Currently, this is
>>>> told by the DT), it says that this is a non-DT environment. On such
>>>> a case, this display bridge driver(it66121.ko) should behavior like
>>>> a *agent*. Because the upstream port of it66121 is the DVO port of
>>>> the display controller, the downstream port of it66121 is the HDMI
>>>> connector. it66121 is on the middle. So I think the it66121.ko should
>>>> handle all of troubles on behalf of the display controller drivers.
>>> No. Don't make decisions for the other drivers. They might have different needs.
>> [...]
>>
>>
>>>> Therefore (when in non-DT use case), the display controller drivers
>>>> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
>>>> Which is to hint that the it66121 should totally in charge of those
>>>> tasks (either by using bridge connector helper or create a connector
>>>> manually). I don't understand on such a case, why bother display
>>>> controller drivers anymore.
>>> This is the reason why we had introduced this flag. It allows the
>>> driver to customise the connector. It even allows the driver to
>>> implement a connector on its own, completely ignoring the
>>> drm_bridge_connector.
>>
>> I know what you said is right in the sense of the universe cases,
>> but I think the most frequent(majority) use case is that there is
>> only one display bridge on the middle. Therefore, I don't want to
>> movethe connector things into device driver if there is only one display
>> bridge(say it66121) in the middle. After all, there is no *direct
>> physical connection* from the perspective of the hardware. I means that
>> there is no hardware wires connectthe HDMI connector and the DVO port. So display controller drivers
>> should not interact with anything related with the connector on a
>> perfect abstract on the software side. Especially for such a simple use
>> case. It probably make senses to make a  decision for themost frequently use case, please also note
>> that this patch didn't introduce any-restriction for the more advance
>> uses cases(multiple bridges in the middle).
> So, for the sake of not having the connector in the display driver,
> you want to add boilerplate code basically to each and every bridge
> driver. In the end, they should all behave in the same way.

No, I'm only intend to modify the one when there has a user emerged.
If we have the connector related code in the KMS display driver side,
then I think we don't honor the meaning of the word *bridge*. I was
told drm_bridge is a modern design, if we still need the DC side
worry about something do not have a physical connection, then it will
not be modern anymore, it is not a good bridge.


> Moreover, there is no way this implementation can work without a
> warning if there are two bridges in a chain and the it66121 is the
> second (the last) one.

Yes and no!

If one of them are transparent, then thisimplementation still can works. It is just that this will not be a good 
abstract anymore.I do have seen such design on some notebook hardware.  But from my programming experiences,
using two bridges are typically a bad practice in engineering. As it tend
to increase the PCB board area and increase entire cost. As you need buy
one more TX encoder chip. Please also consider that the embedded world focus
on low cost and low power consume.

I think, multiple display bridges case should be avoided for middle/low end
application. Or allow us to handle the two and/or more bridges cases in the
future. When there has at least one user emerged, we will introduce new
approach to handle then.

Do you find any product level boards that using two external display bridge and
one of them is it66121? If we can not even find a user, we are not even have a
board to test if current design (state of art) works. Does it suffer from module
loading order problems? what if their i2c slave address is same? Does such a use
case will past the S3/S4 test? All of those concerns are imposed to every display
bridges programmer from the very beginning.

I'm agree with the idea that drm bridges drivers involved toward to a direction
that support more complex design, but I think we should also leave a way for the
most frequent use case. Make it straight-forward as a canonical design.

> The host can not specify the
> DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, it will cause a warning here. And
> it can not omit the flag (as otherwise the first bridge will create a
> connector, without consulting the second bridge).

The semantics of DRM_BRIDGE_ATTACH_NO_CONNECTOR flagare implement-defined, for our case, I could just ignore it if their 
don't have a signal(DT or ACPI) tell me that there are multiple bridges 
in the chain. This depend on community's attitude.

For it66121 with a canonical design, the host should not need to specify this flag.
Because at the time of when the device driver(it66121.ko) get loaded, the it66121
driver could parse the DT by itself, and detect if there has a next bridge, is it a
connector or is it yet another display bridges. The DT speak everything about the
topology. The flag is there just for the KMS display controller driver to explicit
control, use it and make it more useful is the right way, is it?


>>
>>>>>> +
>>>>>> +               connector = drm_bridge_connector_init(bridge->dev, encoder);
>>>>>> +               if (IS_ERR(connector))
>>>>>> +                       return PTR_ERR(connector);
>>>>>> +
>>>>>> +               drm_connector_attach_encoder(connector, encoder);
>>>>> This goes into your device driver.
>>>>>
>>>>>> +
>>>>>> +               ctx->connector = connector;
>>>>>> +       }
>>>>>>
>>>>>>            if (ctx->info->id == ID_IT66121) {
>>>>>>                    ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
>>>>>> @@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
>>>>>>            "vcn33", "vcn18", "vrf12"
>>>>>>     };
>>>
>
>

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Dmitry Baryshkov]

On Thu, 23 Nov 2023 at 07:05, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Hi,
>
>
> On 2023/11/16 19:19, Dmitry Baryshkov wrote:
> > On Thu, 16 Nov 2023 at 12:13, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> >> Hi,
> >>
> >>
> >> On 2023/11/16 17:30, Dmitry Baryshkov wrote:
> >>> On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> >>>> Hi,
> >>>>
> >>>> Thanks a lot for reviewing!
> >>>>
> >>>>
> >>>> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
> >>>>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> >>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
> >>>>>>
> >>>>>> The it66121_create_bridge() and it66121_destroy_bridge() are added to
> >>>>>> export the core functionalities. Create a connector manually by using
> >>>>>> bridge connector helpers when link as a lib.
> >>>>>>
> >>>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> >>>>>> ---
> >>>>>>     drivers/gpu/drm/bridge/ite-it66121.c | 134 +++++++++++++++++++--------
> >>>>>>     include/drm/bridge/ite-it66121.h     |  17 ++++
> >>>>>>     2 files changed, 113 insertions(+), 38 deletions(-)
> >>>>>>     create mode 100644 include/drm/bridge/ite-it66121.h
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> >>>>>> index 8971414a2a60..f5968b679c5d 100644
> >>>>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> >>>>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> >>>>>> @@ -22,6 +22,7 @@
> >>>>>>
> >>>>>>     #include <drm/drm_atomic_helper.h>
> >>>>>>     #include <drm/drm_bridge.h>
> >>>>>> +#include <drm/drm_bridge_connector.h>
> >>>>>>     #include <drm/drm_edid.h>
> >>>>>>     #include <drm/drm_modes.h>
> >>>>>>     #include <drm/drm_print.h>
> >>>>>> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
> >>>>>>                                     enum drm_bridge_attach_flags flags)
> >>>>>>     {
> >>>>>>            struct it66121_ctx *ctx = bridge_to_it66121(bridge);
> >>>>>> +       struct drm_bridge *next_bridge = ctx->next_bridge;
> >>>>>> +       struct drm_encoder *encoder = bridge->encoder;
> >>>>>>            int ret;
> >>>>>>
> >>>>>> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> >>>>>> -               return -EINVAL;
> >>>>>> +       if (next_bridge) {
> >>>>>> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> >>>>>> +                       WARN_ON(1);
> >>>>> Why? At least use WARN() instead
> >>>> Originally I want to
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>>> +                       flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
> >>>>>> +               }
> >>>>>> +               ret = drm_bridge_attach(encoder, next_bridge, bridge, flags);
> >>>>>> +               if (ret)
> >>>>>> +                       return ret;
> >>>>>> +       } else {
> >>>>>> +               struct drm_connector *connector;
> >>>>>>
> >>>>>> -       ret = drm_bridge_attach(bridge->encoder, ctx->next_bridge, bridge, flags);
> >>>>>> -       if (ret)
> >>>>>> -               return ret;
> >>>>>> +               if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> >>>>>> +                       WARN_ON(1);
> >>>>> No. It is perfectly fine to create attach a bridge with no next_bridge
> >>>>> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
> >>>>>
> >>>> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
> >>>> the bridge shall not create a drm_connector. So I think if a display
> >>>> bridge driver don't have a next bridge attached (Currently, this is
> >>>> told by the DT), it says that this is a non-DT environment. On such
> >>>> a case, this display bridge driver(it66121.ko) should behavior like
> >>>> a *agent*. Because the upstream port of it66121 is the DVO port of
> >>>> the display controller, the downstream port of it66121 is the HDMI
> >>>> connector. it66121 is on the middle. So I think the it66121.ko should
> >>>> handle all of troubles on behalf of the display controller drivers.
> >>> No. Don't make decisions for the other drivers. They might have different needs.
> >> [...]
> >>
> >>
> >>>> Therefore (when in non-DT use case), the display controller drivers
> >>>> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
> >>>> Which is to hint that the it66121 should totally in charge of those
> >>>> tasks (either by using bridge connector helper or create a connector
> >>>> manually). I don't understand on such a case, why bother display
> >>>> controller drivers anymore.
> >>> This is the reason why we had introduced this flag. It allows the
> >>> driver to customise the connector. It even allows the driver to
> >>> implement a connector on its own, completely ignoring the
> >>> drm_bridge_connector.
> >>
> >> I know what you said is right in the sense of the universe cases,
> >> but I think the most frequent(majority) use case is that there is
> >> only one display bridge on the middle. Therefore, I don't want to
> >> movethe connector things into device driver if there is only one display
> >> bridge(say it66121) in the middle. After all, there is no *direct
> >> physical connection* from the perspective of the hardware. I means that
> >> there is no hardware wires connectthe HDMI connector and the DVO port. So display controller drivers
> >> should not interact with anything related with the connector on a
> >> perfect abstract on the software side. Especially for such a simple use
> >> case. It probably make senses to make a  decision for themost frequently use case, please also note
> >> that this patch didn't introduce any-restriction for the more advance
> >> uses cases(multiple bridges in the middle).
> > So, for the sake of not having the connector in the display driver,
> > you want to add boilerplate code basically to each and every bridge
> > driver. In the end, they should all behave in the same way.
> >
> > Moreover, there is no way this implementation can work without a
> > warning if there are two bridges in a chain and the it66121 is the
> > second (the last) one. The host can not specify the
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> >>>>>> +                       WARN_ON(1);
> >>>>> No. It is perfectly fine to create attach a bridge with no next_bridge
> >>>>> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
> >>>>>
> >>>> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
> >>>> the bridge shall not create a drm_connector. So I think if a display
> >>>> bridge driver don't have a next bridge attached (Currently, this is
> >>>> told by the DT), it says that this is a non-DT environment. On such
> >>>> a case, this display bridge driver(it66121.ko) should behavior like
> >>>> a *agent*. Because the upstream port of it66121 is the DVO port of
> >>>> the display controller, the downstream port of it66121 is the HDMI
> >>>> connector. it66121 is on the middle. So I think the it66121.ko should
> >>>> handle all of troubles on behalf of the display controller drivers.
> >>> No. Don't make decisions for the other drivers. They might have different needs.
> >> [...]
> >>
> >>
> >>>> Therefore (when in non-DT use case), the display controller drivers
> >>>> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
> >>>> Which is to hint that the it66121 should totally in charge of those
> >>>> tasks (either by using bridge connector helper or create a connector
> >>>> manually). I don't understand on such a case, why bother display
> >>>> controller drivers anymore.
> >>> This is the reason why we had introduced this flag. It allows the
> >>> driver to customise the connector. It even allows the driver to
> >>> implement a connector on its own, completely ignoring the
> >>> drm_bridge_connector.
> >>
> >> I know what you said is right in the sense of the universe cases,
> >> but I think the most frequent(majority) use case is that there is
> >> only one display bridge on the middle. Therefore, I don't want to
> >> movethe connector things into device driver if there is only one display
> >> bridge(say it66121) in the middle. After all, there is no *direct
> >> physical connection* from the perspective of the hardware. I means that
> >> there is no hardware wires connectthe HDMI connector and the DVO port. So display controller drivers
> >> should not interact with anything related with the connector on a
> >> perfect abstract on the software side. Especially for such a simple use
> >> case. It probably make senses to make a  decision for themost frequently use case, please also note
> >> that this patch didn't introduce any-restriction for the more advance
> >> uses cases(multiple bridges in the middle).
> > So, for the sake of not having the connector in the display driver,
> > you want to add boilerplate code basically to each and every bridge
> > driver. In the end, they should all behave in the same way.
>
> No, I'm only intend to modify the one when there has a user emerged.
> If we have the connector related code in the KMS display driver side,
> then I think we don't honor the meaning of the word *bridge*. I was
> told drm_bridge is a modern design, if we still need the DC side
> worry about something do not have a physical connection, then it will
> not be modern anymore, it is not a good bridge.

Next time the user emerges for another bridge. And then for another.
This way the very similar code is copy-pasted over all bridge drivers.
So instead it was decided to keep it in the display driver code.

>
>
> > Moreover, there is no way this implementation can work without a
> > warning if there are two bridges in a chain and the it66121 is the
> > second (the last) one.
>
> Yes and no!
>
> If one of them are transparent, then thisimplementation still can works. It is just that this will not be a good
> abstract anymore.I do have seen such design on some notebook hardware.  But from my programming experiences,
> using two bridges are typically a bad practice in engineering. As it tend
> to increase the PCB board area and increase entire cost. As you need buy
> one more TX encoder chip. Please also consider that the embedded world focus
> on low cost and low power consume.

A typical pipeline for an embedded device can perfectly look like:
- DSI host (drm_encoder)
- DSI-HDMI or DSI-eDP bridge (drm_bridge)
- hdmi-connector or panel-bridge (drm_bridge)
- drm_bridge_connector.

Two drm_bridge instances.

>
> I think, multiple display bridges case should be avoided for middle/low end
> application. Or allow us to handle the two and/or more bridges cases in the
> future. When there has at least one user emerged, we will introduce new
> approach to handle then.
>
> Do you find any product level boards that using two external display bridge and
> one of them is it66121? If we can not even find a user, we are not even have a
> board to test if current design (state of art) works. Does it suffer from module
> loading order problems? what if their i2c slave address is same? Does such a use
> case will past the S3/S4 test? All of those concerns are imposed to every display
> bridges programmer from the very beginning.

Please add a hdmi-connector device to your testing model. You don't
have to use it, but it is a fully legit use case. And suddenly you
have to drm_bridge instances in your chain.

>
> I'm agree with the idea that drm bridges drivers involved toward to a direction
> that support more complex design, but I think we should also leave a way for the
> most frequent use case. Make it straight-forward as a canonical design.

Not having anything connector-related in the drm_bridge driver is a
canonical design.

>
> > The host can not specify the
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, it will cause a warning here. And
> > it can not omit the flag (as otherwise the first bridge will create a
> > connector, without consulting the second bridge).
>
> The semantics of DRM_BRIDGE_ATTACH_NO_CONNECTOR flagare implement-defined,

No, they are not. Semantics are pretty simple: do not create the
drm_connector instance. Pass the flag to the next bridge in the chain.

> for our case, I could just ignore it if their
> don't have a signal(DT or ACPI) tell me that there are multiple bridges
> in the chain. This depend on community's attitude.

Ignoring a flag is a bad idea.

>
> For it66121 with a canonical design, the host should not need to specify this flag.
> Because at the time of when the device driver(it66121.ko) get loaded, the it66121
> driver could parse the DT by itself, and detect if there has a next bridge, is it a
> connector or is it yet another display bridges. The DT speak everything about the
> topology. The flag is there just for the KMS display controller driver to explicit
> control, use it and make it more useful is the right way, is it?

No. We have been there (before the DRM_BRIDGE_ATTACH_NO_CONNECTOR was
introduced), we have gone away from it.

>
>
> >>
> >>>>>> +
> >>>>>> +               connector = drm_bridge_connector_init(bridge->dev, encoder);
> >>>>>> +               if (IS_ERR(connector))
> >>>>>> +                       return PTR_ERR(connector);
> >>>>>> +
> >>>>>> +               drm_connector_attach_encoder(connector, encoder);
> >>>>> This goes into your device driver.
> >>>>>
> >>>>>> +
> >>>>>> +               ctx->connector = connector;
> >>>>>> +       }
> >>>>>>
> >>>>>>            if (ctx->info->id == ID_IT66121) {
> >>>>>>                    ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
> >>>>>> @@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
> >>>>>>            "vcn33", "vcn18", "vrf12"
> >>>>>>     };
> >>>
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Neil Armstrong]

On 23/11/2023 09:08, Dmitry Baryshkov wrote:
> On Thu, 23 Nov 2023 at 07:05, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>
>> Hi,
>>
>>
>> On 2023/11/16 19:19, Dmitry Baryshkov wrote:
>>> On Thu, 16 Nov 2023 at 12:13, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 2023/11/16 17:30, Dmitry Baryshkov wrote:
>>>>> On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Thanks a lot for reviewing!
>>>>>>
>>>>>>
>>>>>> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
>>>>>>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>>>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>>>>
>>>>>>>> The it66121_create_bridge() and it66121_destroy_bridge() are added to
>>>>>>>> export the core functionalities. Create a connector manually by using
>>>>>>>> bridge connector helpers when link as a lib.
>>>>>>>>
>>>>>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/bridge/ite-it66121.c | 134 +++++++++++++++++++--------
>>>>>>>>      include/drm/bridge/ite-it66121.h     |  17 ++++
>>>>>>>>      2 files changed, 113 insertions(+), 38 deletions(-)
>>>>>>>>      create mode 100644 include/drm/bridge/ite-it66121.h
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>>>> index 8971414a2a60..f5968b679c5d 100644
>>>>>>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>>>> @@ -22,6 +22,7 @@
>>>>>>>>
>>>>>>>>      #include <drm/drm_atomic_helper.h>
>>>>>>>>      #include <drm/drm_bridge.h>
>>>>>>>> +#include <drm/drm_bridge_connector.h>
>>>>>>>>      #include <drm/drm_edid.h>
>>>>>>>>      #include <drm/drm_modes.h>
>>>>>>>>      #include <drm/drm_print.h>
>>>>>>>> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
>>>>>>>>                                      enum drm_bridge_attach_flags flags)
>>>>>>>>      {
>>>>>>>>             struct it66121_ctx *ctx = bridge_to_it66121(bridge);
>>>>>>>> +       struct drm_bridge *next_bridge = ctx->next_bridge;
>>>>>>>> +       struct drm_encoder *encoder = bridge->encoder;
>>>>>>>>             int ret;
>>>>>>>>
>>>>>>>> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>>>>>>>> -               return -EINVAL;
>>>>>>>> +       if (next_bridge) {
>>>>>>>> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
>>>>>>>> +                       WARN_ON(1);
>>>>>>> Why? At least use WARN() instead
>>>>>> Originally I want to
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>> +                       flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
>>>>>>>> +               }
>>>>>>>> +               ret = drm_bridge_attach(encoder, next_bridge, bridge, flags);
>>>>>>>> +               if (ret)
>>>>>>>> +                       return ret;
>>>>>>>> +       } else {
>>>>>>>> +               struct drm_connector *connector;
>>>>>>>>
>>>>>>>> -       ret = drm_bridge_attach(bridge->encoder, ctx->next_bridge, bridge, flags);
>>>>>>>> -       if (ret)
>>>>>>>> -               return ret;
>>>>>>>> +               if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
>>>>>>>> +                       WARN_ON(1);
>>>>>>> No. It is perfectly fine to create attach a bridge with no next_bridge
>>>>>>> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
>>>>>>>
>>>>>> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
>>>>>> the bridge shall not create a drm_connector. So I think if a display
>>>>>> bridge driver don't have a next bridge attached (Currently, this is
>>>>>> told by the DT), it says that this is a non-DT environment. On such
>>>>>> a case, this display bridge driver(it66121.ko) should behavior like
>>>>>> a *agent*. Because the upstream port of it66121 is the DVO port of
>>>>>> the display controller, the downstream port of it66121 is the HDMI
>>>>>> connector. it66121 is on the middle. So I think the it66121.ko should
>>>>>> handle all of troubles on behalf of the display controller drivers.
>>>>> No. Don't make decisions for the other drivers. They might have different needs.
>>>> [...]
>>>>
>>>>
>>>>>> Therefore (when in non-DT use case), the display controller drivers
>>>>>> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
>>>>>> Which is to hint that the it66121 should totally in charge of those
>>>>>> tasks (either by using bridge connector helper or create a connector
>>>>>> manually). I don't understand on such a case, why bother display
>>>>>> controller drivers anymore.
>>>>> This is the reason why we had introduced this flag. It allows the
>>>>> driver to customise the connector. It even allows the driver to
>>>>> implement a connector on its own, completely ignoring the
>>>>> drm_bridge_connector.
>>>>
>>>> I know what you said is right in the sense of the universe cases,
>>>> but I think the most frequent(majority) use case is that there is
>>>> only one display bridge on the middle. Therefore, I don't want to
>>>> movethe connector things into device driver if there is only one display
>>>> bridge(say it66121) in the middle. After all, there is no *direct
>>>> physical connection* from the perspective of the hardware. I means that
>>>> there is no hardware wires connectthe HDMI connector and the DVO port. So display controller drivers
>>>> should not interact with anything related with the connector on a
>>>> perfect abstract on the software side. Especially for such a simple use
>>>> case. It probably make senses to make a  decision for themost frequently use case, please also note
>>>> that this patch didn't introduce any-restriction for the more advance
>>>> uses cases(multiple bridges in the middle).
>>> So, for the sake of not having the connector in the display driver,
>>> you want to add boilerplate code basically to each and every bridge
>>> driver. In the end, they should all behave in the same way.
>>>
>>> Moreover, there is no way this implementation can work without a
>>> warning if there are two bridges in a chain and the it66121 is the
>>> second (the last) one. The host can not specify the
>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR)
>>>>>>>> +                       WARN_ON(1);
>>>>>>> No. It is perfectly fine to create attach a bridge with no next_bridge
>>>>>>> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
>>>>>>>
>>>>>> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
>>>>>> the bridge shall not create a drm_connector. So I think if a display
>>>>>> bridge driver don't have a next bridge attached (Currently, this is
>>>>>> told by the DT), it says that this is a non-DT environment. On such
>>>>>> a case, this display bridge driver(it66121.ko) should behavior like
>>>>>> a *agent*. Because the upstream port of it66121 is the DVO port of
>>>>>> the display controller, the downstream port of it66121 is the HDMI
>>>>>> connector. it66121 is on the middle. So I think the it66121.ko should
>>>>>> handle all of troubles on behalf of the display controller drivers.
>>>>> No. Don't make decisions for the other drivers. They might have different needs.
>>>> [...]
>>>>
>>>>
>>>>>> Therefore (when in non-DT use case), the display controller drivers
>>>>>> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
>>>>>> Which is to hint that the it66121 should totally in charge of those
>>>>>> tasks (either by using bridge connector helper or create a connector
>>>>>> manually). I don't understand on such a case, why bother display
>>>>>> controller drivers anymore.
>>>>> This is the reason why we had introduced this flag. It allows the
>>>>> driver to customise the connector. It even allows the driver to
>>>>> implement a connector on its own, completely ignoring the
>>>>> drm_bridge_connector.
>>>>
>>>> I know what you said is right in the sense of the universe cases,
>>>> but I think the most frequent(majority) use case is that there is
>>>> only one display bridge on the middle. Therefore, I don't want to
>>>> movethe connector things into device driver if there is only one display
>>>> bridge(say it66121) in the middle. After all, there is no *direct
>>>> physical connection* from the perspective of the hardware. I means that
>>>> there is no hardware wires connectthe HDMI connector and the DVO port. So display controller drivers
>>>> should not interact with anything related with the connector on a
>>>> perfect abstract on the software side. Especially for such a simple use
>>>> case. It probably make senses to make a  decision for themost frequently use case, please also note
>>>> that this patch didn't introduce any-restriction for the more advance
>>>> uses cases(multiple bridges in the middle).
>>> So, for the sake of not having the connector in the display driver,
>>> you want to add boilerplate code basically to each and every bridge
>>> driver. In the end, they should all behave in the same way.
>>
>> No, I'm only intend to modify the one when there has a user emerged.
>> If we have the connector related code in the KMS display driver side,
>> then I think we don't honor the meaning of the word *bridge*. I was
>> told drm_bridge is a modern design, if we still need the DC side
>> worry about something do not have a physical connection, then it will
>> not be modern anymore, it is not a good bridge.
> 
> Next time the user emerges for another bridge. And then for another.
> This way the very similar code is copy-pasted over all bridge drivers.
> So instead it was decided to keep it in the display driver code.
> 
>>
>>
>>> Moreover, there is no way this implementation can work without a
>>> warning if there are two bridges in a chain and the it66121 is the
>>> second (the last) one.
>>
>> Yes and no!
>>
>> If one of them are transparent, then thisimplementation still can works. It is just that this will not be a good
>> abstract anymore.I do have seen such design on some notebook hardware.  But from my programming experiences,
>> using two bridges are typically a bad practice in engineering. As it tend
>> to increase the PCB board area and increase entire cost. As you need buy
>> one more TX encoder chip. Please also consider that the embedded world focus
>> on low cost and low power consume.
> 
> A typical pipeline for an embedded device can perfectly look like:
> - DSI host (drm_encoder)
> - DSI-HDMI or DSI-eDP bridge (drm_bridge)
> - hdmi-connector or panel-bridge (drm_bridge)
> - drm_bridge_connector.
> 
> Two drm_bridge instances.

Nowadays, we are moving the encoder code into bridge so we can have way
more than 2 bridges, and connector code has been moved to a bridge.

On Amlogic SoCs, the HDMI chain has at least 3 bridges, it can have up to 4
on DSI usecase if you plug a DSI to eDP bridge.

On iMX8 platform, they have multiple internal SoC bridges even before the HDMI or DSI bridge.

The model works very well across extremely heterogeneous embedded platforms.

> 
>>
>> I think, multiple display bridges case should be avoided for middle/low end
>> application. Or allow us to handle the two and/or more bridges cases in the
>> future. When there has at least one user emerged, we will introduce new
>> approach to handle then.
>>
>> Do you find any product level boards that using two external display bridge and
>> one of them is it66121? If we can not even find a user, we are not even have a
>> board to test if current design (state of art) works. Does it suffer from module
>> loading order problems? what if their i2c slave address is same? Does such a use
>> case will past the S3/S4 test? All of those concerns are imposed to every display
>> bridges programmer from the very beginning.
> 
> Please add a hdmi-connector device to your testing model. You don't
> have to use it, but it is a fully legit use case. And suddenly you
> have to drm_bridge instances in your chain.
> 
>>
>> I'm agree with the idea that drm bridges drivers involved toward to a direction
>> that support more complex design, but I think we should also leave a way for the
>> most frequent use case. Make it straight-forward as a canonical design.
> 
> Not having anything connector-related in the drm_bridge driver is a
> canonical design.
> 
>>
>>> The host can not specify the
>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, it will cause a warning here. And
>>> it can not omit the flag (as otherwise the first bridge will create a
>>> connector, without consulting the second bridge).
>>
>> The semantics of DRM_BRIDGE_ATTACH_NO_CONNECTOR flagare implement-defined,
> 
> No, they are not. Semantics are pretty simple: do not create the
> drm_connector instance. Pass the flag to the next bridge in the chain.
> 
>> for our case, I could just ignore it if their
>> don't have a signal(DT or ACPI) tell me that there are multiple bridges
>> in the chain. This depend on community's attitude.
> 
> Ignoring a flag is a bad idea.
> 
>>
>> For it66121 with a canonical design, the host should not need to specify this flag.
>> Because at the time of when the device driver(it66121.ko) get loaded, the it66121
>> driver could parse the DT by itself, and detect if there has a next bridge, is it a
>> connector or is it yet another display bridges. The DT speak everything about the
>> topology. The flag is there just for the KMS display controller driver to explicit
>> control, use it and make it more useful is the right way, is it?
> 
> No. We have been there (before the DRM_BRIDGE_ATTACH_NO_CONNECTOR was
> introduced), we have gone away from it.
> 
>>
>>
>>>>
>>>>>>>> +
>>>>>>>> +               connector = drm_bridge_connector_init(bridge->dev, encoder);
>>>>>>>> +               if (IS_ERR(connector))
>>>>>>>> +                       return PTR_ERR(connector);
>>>>>>>> +
>>>>>>>> +               drm_connector_attach_encoder(connector, encoder);
>>>>>>> This goes into your device driver.
>>>>>>>
>>>>>>>> +
>>>>>>>> +               ctx->connector = connector;
>>>>>>>> +       }
>>>>>>>>
>>>>>>>>             if (ctx->info->id == ID_IT66121) {
>>>>>>>>                     ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
>>>>>>>> @@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
>>>>>>>>             "vcn33", "vcn18", "vrf12"
>>>>>>>>      };
>>>>>
>>>
>>>
> 
> 
> 

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,


On 2023/11/23 21:39, Neil Armstrong wrote:
> On 23/11/2023 09:08, Dmitry Baryshkov wrote:
>> On Thu, 23 Nov 2023 at 07:05, Sui Jingfeng <sui.jingfeng@linux.dev> 
>> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 2023/11/16 19:19, Dmitry Baryshkov wrote:
>>>> On Thu, 16 Nov 2023 at 12:13, Sui Jingfeng <sui.jingfeng@linux.dev> 
>>>> wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 2023/11/16 17:30, Dmitry Baryshkov wrote:
>>>>>> On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng 
>>>>>> <sui.jingfeng@linux.dev> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Thanks a lot for reviewing!
>>>>>>>
>>>>>>>
>>>>>>> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
>>>>>>>> On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng 
>>>>>>>> <sui.jingfeng@linux.dev> wrote:
>>>>>>>>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>>>>>
>>>>>>>>> The it66121_create_bridge() and it66121_destroy_bridge() are 
>>>>>>>>> added to
>>>>>>>>> export the core functionalities. Create a connector manually 
>>>>>>>>> by using
>>>>>>>>> bridge connector helpers when link as a lib.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>>>>>>> ---
>>>>>>>>>      drivers/gpu/drm/bridge/ite-it66121.c | 134 
>>>>>>>>> +++++++++++++++++++--------
>>>>>>>>>      include/drm/bridge/ite-it66121.h     |  17 ++++
>>>>>>>>>      2 files changed, 113 insertions(+), 38 deletions(-)
>>>>>>>>>      create mode 100644 include/drm/bridge/ite-it66121.h
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c 
>>>>>>>>> b/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>>>>> index 8971414a2a60..f5968b679c5d 100644
>>>>>>>>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>>>>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
>>>>>>>>> @@ -22,6 +22,7 @@
>>>>>>>>>
>>>>>>>>>      #include <drm/drm_atomic_helper.h>
>>>>>>>>>      #include <drm/drm_bridge.h>
>>>>>>>>> +#include <drm/drm_bridge_connector.h>
>>>>>>>>>      #include <drm/drm_edid.h>
>>>>>>>>>      #include <drm/drm_modes.h>
>>>>>>>>>      #include <drm/drm_print.h>
>>>>>>>>> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct 
>>>>>>>>> drm_bridge *bridge,
>>>>>>>>>                                      enum 
>>>>>>>>> drm_bridge_attach_flags flags)
>>>>>>>>>      {
>>>>>>>>>             struct it66121_ctx *ctx = bridge_to_it66121(bridge);
>>>>>>>>> +       struct drm_bridge *next_bridge = ctx->next_bridge;
>>>>>>>>> +       struct drm_encoder *encoder = bridge->encoder;
>>>>>>>>>             int ret;
>>>>>>>>>
>>>>>>>>> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>>>>>>>>> -               return -EINVAL;
>>>>>>>>> +       if (next_bridge) {
>>>>>>>>> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
>>>>>>>>> +                       WARN_ON(1);
>>>>>>>> Why? At least use WARN() instead
>>>>>>> Originally I want to
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>> + flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
>>>>>>>>> +               }
>>>>>>>>> +               ret = drm_bridge_attach(encoder, next_bridge, 
>>>>>>>>> bridge, flags);
>>>>>>>>> +               if (ret)
>>>>>>>>> +                       return ret;
>>>>>>>>> +       } else {
>>>>>>>>> +               struct drm_connector *connector;
>>>>>>>>>
>>>>>>>>> -       ret = drm_bridge_attach(bridge->encoder, 
>>>>>>>>> ctx->next_bridge, bridge, flags);
>>>>>>>>> -       if (ret)
>>>>>>>>> -               return ret;
>>>>>>>>> +               if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
>>>>>>>>> +                       WARN_ON(1);
>>>>>>>> No. It is perfectly fine to create attach a bridge with no 
>>>>>>>> next_bridge
>>>>>>>> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
>>>>>>>>
>>>>>>> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
>>>>>>> the bridge shall not create a drm_connector. So I think if a 
>>>>>>> display
>>>>>>> bridge driver don't have a next bridge attached (Currently, this is
>>>>>>> told by the DT), it says that this is a non-DT environment. On such
>>>>>>> a case, this display bridge driver(it66121.ko) should behavior like
>>>>>>> a *agent*. Because the upstream port of it66121 is the DVO port of
>>>>>>> the display controller, the downstream port of it66121 is the HDMI
>>>>>>> connector. it66121 is on the middle. So I think the it66121.ko 
>>>>>>> should
>>>>>>> handle all of troubles on behalf of the display controller drivers.
>>>>>> No. Don't make decisions for the other drivers. They might have 
>>>>>> different needs.
>>>>> [...]
>>>>>
>>>>>
>>>>>>> Therefore (when in non-DT use case), the display controller drivers
>>>>>>> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
>>>>>>> Which is to hint that the it66121 should totally in charge of those
>>>>>>> tasks (either by using bridge connector helper or create a 
>>>>>>> connector
>>>>>>> manually). I don't understand on such a case, why bother display
>>>>>>> controller drivers anymore.
>>>>>> This is the reason why we had introduced this flag. It allows the
>>>>>> driver to customise the connector. It even allows the driver to
>>>>>> implement a connector on its own, completely ignoring the
>>>>>> drm_bridge_connector.
>>>>>
>>>>> I know what you said is right in the sense of the universe cases,
>>>>> but I think the most frequent(majority) use case is that there is
>>>>> only one display bridge on the middle. Therefore, I don't want to
>>>>> movethe connector things into device driver if there is only one 
>>>>> display
>>>>> bridge(say it66121) in the middle. After all, there is no *direct
>>>>> physical connection* from the perspective of the hardware. I means 
>>>>> that
>>>>> there is no hardware wires connectthe HDMI connector and the DVO 
>>>>> port. So display controller drivers
>>>>> should not interact with anything related with the connector on a
>>>>> perfect abstract on the software side. Especially for such a 
>>>>> simple use
>>>>> case. It probably make senses to make a  decision for themost 
>>>>> frequently use case, please also note
>>>>> that this patch didn't introduce any-restriction for the more advance
>>>>> uses cases(multiple bridges in the middle).
>>>> So, for the sake of not having the connector in the display driver,
>>>> you want to add boilerplate code basically to each and every bridge
>>>> driver. In the end, they should all behave in the same way.
>>>>
>>>> Moreover, there is no way this implementation can work without a
>>>> warning if there are two bridges in a chain and the it66121 is the
>>>> second (the last) one. The host can not specify the
>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR)
>>>>>>>>> + WARN_ON(1);
>>>>>>>> No. It is perfectly fine to create attach a bridge with no 
>>>>>>>> next_bridge
>>>>>>>> and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
>>>>>>>>
>>>>>>> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
>>>>>>> the bridge shall not create a drm_connector. So I think if a 
>>>>>>> display
>>>>>>> bridge driver don't have a next bridge attached (Currently, this is
>>>>>>> told by the DT), it says that this is a non-DT environment. On such
>>>>>>> a case, this display bridge driver(it66121.ko) should behavior like
>>>>>>> a *agent*. Because the upstream port of it66121 is the DVO port of
>>>>>>> the display controller, the downstream port of it66121 is the HDMI
>>>>>>> connector. it66121 is on the middle. So I think the it66121.ko 
>>>>>>> should
>>>>>>> handle all of troubles on behalf of the display controller drivers.
>>>>>> No. Don't make decisions for the other drivers. They might have 
>>>>>> different needs.
>>>>> [...]
>>>>>
>>>>>
>>>>>>> Therefore (when in non-DT use case), the display controller drivers
>>>>>>> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
>>>>>>> Which is to hint that the it66121 should totally in charge of those
>>>>>>> tasks (either by using bridge connector helper or create a 
>>>>>>> connector
>>>>>>> manually). I don't understand on such a case, why bother display
>>>>>>> controller drivers anymore.
>>>>>> This is the reason why we had introduced this flag. It allows the
>>>>>> driver to customise the connector. It even allows the driver to
>>>>>> implement a connector on its own, completely ignoring the
>>>>>> drm_bridge_connector.
>>>>>
>>>>> I know what you said is right in the sense of the universe cases,
>>>>> but I think the most frequent(majority) use case is that there is
>>>>> only one display bridge on the middle. Therefore, I don't want to
>>>>> movethe connector things into device driver if there is only one 
>>>>> display
>>>>> bridge(say it66121) in the middle. After all, there is no *direct
>>>>> physical connection* from the perspective of the hardware. I means 
>>>>> that
>>>>> there is no hardware wires connectthe HDMI connector and the DVO 
>>>>> port. So display controller drivers
>>>>> should not interact with anything related with the connector on a
>>>>> perfect abstract on the software side. Especially for such a 
>>>>> simple use
>>>>> case. It probably make senses to make a  decision for themost 
>>>>> frequently use case, please also note
>>>>> that this patch didn't introduce any-restriction for the more advance
>>>>> uses cases(multiple bridges in the middle).
>>>> So, for the sake of not having the connector in the display driver,
>>>> you want to add boilerplate code basically to each and every bridge
>>>> driver. In the end, they should all behave in the same way.
>>>
>>> No, I'm only intend to modify the one when there has a user emerged.
>>> If we have the connector related code in the KMS display driver side,
>>> then I think we don't honor the meaning of the word *bridge*. I was
>>> told drm_bridge is a modern design, if we still need the DC side
>>> worry about something do not have a physical connection, then it will
>>> not be modern anymore, it is not a good bridge.
>>
>> Next time the user emerges for another bridge. And then for another.
>> This way the very similar code is copy-pasted over all bridge drivers.
>> So instead it was decided to keep it in the display driver code.
>>
>>>
>>>
>>>> Moreover, there is no way this implementation can work without a
>>>> warning if there are two bridges in a chain and the it66121 is the
>>>> second (the last) one.
>>>
>>> Yes and no!
>>>
>>> If one of them are transparent, then thisimplementation still can 
>>> works. It is just that this will not be a good
>>> abstract anymore.I do have seen such design on some notebook 
>>> hardware.  But from my programming experiences,
>>> using two bridges are typically a bad practice in engineering. As it 
>>> tend
>>> to increase the PCB board area and increase entire cost. As you need 
>>> buy
>>> one more TX encoder chip. Please also consider that the embedded 
>>> world focus
>>> on low cost and low power consume.
>>
>> A typical pipeline for an embedded device can perfectly look like:
>> - DSI host (drm_encoder)
>> - DSI-HDMI or DSI-eDP bridge (drm_bridge)
>> - hdmi-connector or panel-bridge (drm_bridge)
>> - drm_bridge_connector.
>>
>> Two drm_bridge instances.
>
> Nowadays, we are moving the encoder code into bridge so we can have way
> more than 2 bridges, and connector code has been moved to a bridge.
>
> On Amlogic SoCs, the HDMI chain has at least 3 bridges, it can have up 
> to 4
> on DSI usecase if you plug a DSI to eDP bridge.
>
> On iMX8 platform, they have multiple internal SoC bridges even before 
> the HDMI or DSI bridge.
>
> The model works very well across extremely heterogeneous embedded 
> platforms.
>
But you don't mention the prerequisite, there have difference between
display bridge devices *inside* of the SoC and display bridge devices
*outside* of the SoC. IT66121 is a onboard device. Suppose you are the
hardware vendor ITE, you definitely want your chip can be used on any
platform. The model probably works well on ARM, but on X86 platform
there no DT support, the abstract for the onboard device becomes pale.


>>
>>>
>>> I think, multiple display bridges case should be avoided for 
>>> middle/low end
>>> application. Or allow us to handle the two and/or more bridges cases 
>>> in the
>>> future. When there has at least one user emerged, we will introduce new
>>> approach to handle then.
>>>
>>> Do you find any product level boards that using two external display 
>>> bridge and
>>> one of them is it66121? If we can not even find a user, we are not 
>>> even have a
>>> board to test if current design (state of art) works. Does it suffer 
>>> from module
>>> loading order problems? what if their i2c slave address is same? 
>>> Does such a use
>>> case will past the S3/S4 test? All of those concerns are imposed to 
>>> every display
>>> bridges programmer from the very beginning.
>>
>> Please add a hdmi-connector device to your testing model. You don't
>> have to use it, but it is a fully legit use case. And suddenly you
>> have to drm_bridge instances in your chain.
>>
>>>
>>> I'm agree with the idea that drm bridges drivers involved toward to 
>>> a direction
>>> that support more complex design, but I think we should also leave a 
>>> way for the
>>> most frequent use case. Make it straight-forward as a canonical design.
>>
>> Not having anything connector-related in the drm_bridge driver is a
>> canonical design.
>>
>>>
>>>> The host can not specify the
>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, it will cause a warning here. And
>>>> it can not omit the flag (as otherwise the first bridge will create a
>>>> connector, without consulting the second bridge).
>>>
>>> The semantics of DRM_BRIDGE_ATTACH_NO_CONNECTOR flagare 
>>> implement-defined,
>>
>> No, they are not. Semantics are pretty simple: do not create the
>> drm_connector instance. Pass the flag to the next bridge in the chain.
>>
>>> for our case, I could just ignore it if their
>>> don't have a signal(DT or ACPI) tell me that there are multiple bridges
>>> in the chain. This depend on community's attitude.
>>
>> Ignoring a flag is a bad idea.
>>
>>>
>>> For it66121 with a canonical design, the host should not need to 
>>> specify this flag.
>>> Because at the time of when the device driver(it66121.ko) get 
>>> loaded, the it66121
>>> driver could parse the DT by itself, and detect if there has a next 
>>> bridge, is it a
>>> connector or is it yet another display bridges. The DT speak 
>>> everything about the
>>> topology. The flag is there just for the KMS display controller 
>>> driver to explicit
>>> control, use it and make it more useful is the right way, is it?
>>
>> No. We have been there (before the DRM_BRIDGE_ATTACH_NO_CONNECTOR was
>> introduced), we have gone away from it.
>>
>>>
>>>
>>>>>
>>>>>>>>> +
>>>>>>>>> +               connector = 
>>>>>>>>> drm_bridge_connector_init(bridge->dev, encoder);
>>>>>>>>> +               if (IS_ERR(connector))
>>>>>>>>> +                       return PTR_ERR(connector);
>>>>>>>>> +
>>>>>>>>> + drm_connector_attach_encoder(connector, encoder);
>>>>>>>> This goes into your device driver.
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +               ctx->connector = connector;
>>>>>>>>> +       }
>>>>>>>>>
>>>>>>>>>             if (ctx->info->id == ID_IT66121) {
>>>>>>>>>                     ret = regmap_write_bits(ctx->regmap, 
>>>>>>>>> IT66121_CLK_BANK_REG,
>>>>>>>>> @@ -1632,16 +1651,13 @@ static const char * const 
>>>>>>>>> it66121_supplies[] = {
>>>>>>>>>             "vcn33", "vcn18", "vrf12"
>>>>>>>>>      };
>>>>>>
>>>>
>>>>
>>
>>
>>
>

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,


On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>> The semantics of DRM_BRIDGE_ATTACH_NO_CONNECTOR flag are implement-defined,
> No, they are not. Semantics are pretty simple: do not create the
> drm_connector instance. Pass the flag to the next bridge in the chain.


Then, my problem is that do we allow create a drm_connector instance in drm bridge
driver nowadays?

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Dmitry Baryshkov]

On Thu, 23 Nov 2023 at 17:41, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Hi,
>
>
> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
> >> The semantics of DRM_BRIDGE_ATTACH_NO_CONNECTOR flag are implement-defined,
> > No, they are not. Semantics are pretty simple: do not create the
> > drm_connector instance. Pass the flag to the next bridge in the chain.
>
>
> Then, my problem is that do we allow create a drm_connector instance in drm bridge
> driver nowadays?

Yes, if there is no DRM_BRIDGE_ATTACH_NO_CONNECTOR. But that's deprecated IMO.

-- 
With best wishes
Dmitry

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,

On 2023/11/24 00:06, Dmitry Baryshkov wrote:
> On Thu, 23 Nov 2023 at 17:41, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>> Hi,
>>
>>
>> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>>>> The semantics of DRM_BRIDGE_ATTACH_NO_CONNECTOR flag are implement-defined,
>>> No, they are not. Semantics are pretty simple: do not create the
>>> drm_connector instance. Pass the flag to the next bridge in the chain.
>>
>> Then, my problem is that do we allow create a drm_connector instance in drm bridge
>> driver nowadays?
> Yes, if there is no DRM_BRIDGE_ATTACH_NO_CONNECTOR. But that's deprecated IMO.

Then can you read the code in bridge/ti-tfp410.c please?
The tfp410_attach() function will create drm_connector instance manually
if the conditional (flags == 0) is true.

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,


On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>>> The host can not specify the
>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, it will cause a warning here. And
>>> it can not omit the flag (as otherwise the first bridge will create a
>>> connector, without consulting the second bridge).
>> The semantics of DRM_BRIDGE_ATTACH_NO_CONNECTOR flagare implement-defined,
> No, they are not. Semantics are pretty simple: do not create the
> drm_connector instance. Pass the flag to the next bridge in the chain.
>
>> for our case, I could just ignore it if their
>> don't have a signal(DT or ACPI) tell me that there are multiple bridges
>> in the chain. This depend on community's attitude.
> Ignoring a flag is a bad idea.


Can you also read the code in the bridge/lontium-lt8912.c please?
when flags == 0 is true, the lt8912 driver will just create
a drm_connector instance in the drm bridge drivers. The behavior
is similar with this patch in the perspective of spirit.

And the most important thing is that no matter what the flag the upstream
port is passed, lt8912 just always pass the DRM_BRIDGE_ATTACH_NO_CONNECTOR
flags to the next bridge. Does this count as a kind of ignore? or

This is to say that both the lt8912 and the tfp410 drm bridge drivers are
allowing create a drm_connector manually in drm bridge drivers. They didn't
being asked to move the drm_connector related code to display controller
driver. I don't know why I can't follow this way?

Do you really read the code before do comments or I failed to understand something?

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Dmitry Baryshkov]

On Thu, 23 Nov 2023 at 19:04, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Hi,
>
>
> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
> >>> The host can not specify the
> >>> DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, it will cause a warning here. And
> >>> it can not omit the flag (as otherwise the first bridge will create a
> >>> connector, without consulting the second bridge).
> >> The semantics of DRM_BRIDGE_ATTACH_NO_CONNECTOR flagare implement-defined,
> > No, they are not. Semantics are pretty simple: do not create the
> > drm_connector instance. Pass the flag to the next bridge in the chain.
> >
> >> for our case, I could just ignore it if their
> >> don't have a signal(DT or ACPI) tell me that there are multiple bridges
> >> in the chain. This depend on community's attitude.
> > Ignoring a flag is a bad idea.
>
>
> Can you also read the code in the bridge/lontium-lt8912.c please?
> when flags == 0 is true, the lt8912 driver will just create
> a drm_connector instance in the drm bridge drivers. The behavior
> is similar with this patch in the perspective of spirit.
>
> And the most important thing is that no matter what the flag the upstream
> port is passed, lt8912 just always pass the DRM_BRIDGE_ATTACH_NO_CONNECTOR
> flags to the next bridge. Does this count as a kind of ignore? or
>
> This is to say that both the lt8912 and the tfp410 drm bridge drivers are
> allowing create a drm_connector manually in drm bridge drivers. They didn't
> being asked to move the drm_connector related code to display controller
> driver. I don't know why I can't follow this way?

This is called 'legacy'.

>
> Do you really read the code before do comments or I failed to understand something?


-- 
With best wishes
Dmitry

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,


On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>> I'm agree with the idea that drm bridges drivers involved toward to a direction
>> that support more complex design, but I think we should also leave a way for the
>> most frequent use case. Make it straight-forward as a canonical design.
> Not having anything connector-related in the drm_bridge driver is a
> canonical design.

What you said is just for the more complex uses case. I can't agree, sorry.

By choosing the word "canonical design", I means that the most frequently used
cases in practice are the canonical design, 95+% motherboards I have seen has
only one *onboard* display bridges chip. For my driver, I abstract the internal
(inside of the chip) encoder as drm_encoder and abstract the external TX chip as
drm_bridge, this design still works very well.


Originally, I means that this is a concept of the hardware design.
You are wrong even through in the software design context, the
transparent simple drm bridge drivers(simple-bridge.c) also *allow*
to create drm connector manually. I don't think I need to emulate
more example, please read the code by youself.

Canonical or not canonical is not a question to argue, if other programmers
are allowed to do such kind of abstraction, I should also allowed too. Thanks.

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1508 bytes --]

On Fri, Nov 24, 2023 at 01:52:26AM +0800, Sui Jingfeng wrote:
> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
> > > I'm agree with the idea that drm bridges drivers involved toward to a direction
> > > that support more complex design, but I think we should also leave a way for the
> > > most frequent use case. Make it straight-forward as a canonical design.
> > Not having anything connector-related in the drm_bridge driver is a
> > canonical design.
> 
> What you said is just for the more complex uses case. I can't agree, sorry.
> 
> By choosing the word "canonical design", I means that the most frequently used
> cases in practice are the canonical design, 95+% motherboards I have seen has
> only one *onboard* display bridges chip. For my driver, I abstract the internal
> (inside of the chip) encoder as drm_encoder and abstract the external TX chip as
> drm_bridge, this design still works very well.
> 
> 
> Originally, I means that this is a concept of the hardware design.
> You are wrong even through in the software design context, the
> transparent simple drm bridge drivers(simple-bridge.c) also *allow*
> to create drm connector manually. I don't think I need to emulate
> more example, please read the code by youself.

Ok. That's it. We've been patient long enough. You have been given a
review and a list of things to fix for your driver to be merged. Whether
you follow them or not is your decision.

We won't tolerate insulting comments though.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,

On 2023/11/24 15:38, Maxime Ripard wrote:
> On Fri, Nov 24, 2023 at 01:52:26AM +0800, Sui Jingfeng wrote:
>> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>>>> I'm agree with the idea that drm bridges drivers involved toward to a direction
>>>> that support more complex design, but I think we should also leave a way for the
>>>> most frequent use case. Make it straight-forward as a canonical design.
>>> Not having anything connector-related in the drm_bridge driver is a
>>> canonical design.
>> What you said is just for the more complex uses case. I can't agree, sorry.
>>
>> By choosing the word "canonical design", I means that the most frequently used
>> cases in practice are the canonical design, 95+% motherboards I have seen has
>> only one *onboard* display bridges chip. For my driver, I abstract the internal
>> (inside of the chip) encoder as drm_encoder and abstract the external TX chip as
>> drm_bridge, this design still works very well.
>>
>>
>> Originally, I means that this is a concept of the hardware design.
>> You are wrong even through in the software design context, the
>> transparent simple drm bridge drivers(simple-bridge.c) also *allow*
>> to create drm connector manually. I don't think I need to emulate
>> more example, please read the code by youself.

'emulate' -> 'enumerate'


> Ok. That's it. We've been patient long enough. You have been given a
> review and a list of things to fix for your driver to be merged.


This series is not relevant to my driver, can we please *limit*
the discussion to this series?


>   Whether
> you follow them or not is your decision.


I'm not saying that I will not follow, just to make sure what's solution is you want.
I need discussion to figure out.


> We won't tolerate insulting comments though.


There is *no* insulting, please don't misunderstanding before *sufficient* communication, OK?
Originally, I thought Dmitry may ignore(or overlook) what is the current status.


> Maxime

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2743 bytes --]

On Fri, Nov 24, 2023 at 03:51:00PM +0800, Sui Jingfeng wrote:
> Hi,
> 
> On 2023/11/24 15:38, Maxime Ripard wrote:
> > On Fri, Nov 24, 2023 at 01:52:26AM +0800, Sui Jingfeng wrote:
> > > On 2023/11/23 16:08, Dmitry Baryshkov wrote:
> > > > > I'm agree with the idea that drm bridges drivers involved toward to a direction
> > > > > that support more complex design, but I think we should also leave a way for the
> > > > > most frequent use case. Make it straight-forward as a canonical design.
> > > > Not having anything connector-related in the drm_bridge driver is a
> > > > canonical design.
> > > What you said is just for the more complex uses case. I can't agree, sorry.
> > > 
> > > By choosing the word "canonical design", I means that the most frequently used
> > > cases in practice are the canonical design, 95+% motherboards I have seen has
> > > only one *onboard* display bridges chip. For my driver, I abstract the internal
> > > (inside of the chip) encoder as drm_encoder and abstract the external TX chip as
> > > drm_bridge, this design still works very well.
> > > 
> > > 
> > > Originally, I means that this is a concept of the hardware design.
> > > You are wrong even through in the software design context, the
> > > transparent simple drm bridge drivers(simple-bridge.c) also *allow*
> > > to create drm connector manually. I don't think I need to emulate
> > > more example, please read the code by youself.
> 
> 'emulate' -> 'enumerate'
>
> > Ok. That's it. We've been patient long enough. You have been given a
> > review and a list of things to fix for your driver to be merged.
>
> This series is not relevant to my driver, can we please *limit* the
> discussion to this series?

Right, I conflated the two, I meant this series, or the general goal to
enable that bridge with your driver. The rest of the driver is of course
unaffected.

> > Whether you follow them or not is your decision.
>
> I'm not saying that I will not follow, just to make sure what's
> solution is you want. I need discussion to figure out.

You had direct, repeated, feedback on that already by a maintainer and
one of the most experienced dev and reviewer on bridges. If you need
more guidance, you can definitely ask questions, but asking questions
and telling them they are wrong is very different.

> > We won't tolerate insulting comments though.
>
> There is *no* insulting, please don't misunderstanding before
> *sufficient* communication, OK? Originally, I thought Dmitry may
> ignore(or overlook) what is the current status.

Saying to someone maintaining and/or reviewing that code for years now
that they are wrong and should go read the code is insulting.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,

On 2023/11/24 16:13, Maxime Ripard wrote:
> On Fri, Nov 24, 2023 at 03:51:00PM +0800, Sui Jingfeng wrote:
>> Hi,
>>
>> On 2023/11/24 15:38, Maxime Ripard wrote:
>>> On Fri, Nov 24, 2023 at 01:52:26AM +0800, Sui Jingfeng wrote:
>>>> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>>>>>> I'm agree with the idea that drm bridges drivers involved toward to a direction
>>>>>> that support more complex design, but I think we should also leave a way for the
>>>>>> most frequent use case. Make it straight-forward as a canonical design.
>>>>> Not having anything connector-related in the drm_bridge driver is a
>>>>> canonical design.
>>>> What you said is just for the more complex uses case. I can't agree, sorry.
>>>>
>>>> By choosing the word "canonical design", I means that the most frequently used
>>>> cases in practice are the canonical design, 95+% motherboards I have seen has
>>>> only one *onboard* display bridges chip. For my driver, I abstract the internal
>>>> (inside of the chip) encoder as drm_encoder and abstract the external TX chip as
>>>> drm_bridge, this design still works very well.
>>>>
>>>>
>>>> Originally, I means that this is a concept of the hardware design.
>>>> You are wrong even through in the software design context, the
>>>> transparent simple drm bridge drivers(simple-bridge.c) also *allow*
>>>> to create drm connector manually. I don't think I need to emulate
>>>> more example, please read the code by youself.
>> 'emulate' -> 'enumerate'
>>
>>> Ok. That's it. We've been patient long enough. You have been given a
>>> review and a list of things to fix for your driver to be merged.
>> This series is not relevant to my driver, can we please *limit* the
>> discussion to this series?
> Right, I conflated the two, I meant this series, or the general goal to
> enable that bridge with your driver. The rest of the driver is of course
> unaffected.
>
>>> Whether you follow them or not is your decision.
>> I'm not saying that I will not follow, just to make sure what's
>> solution is you want. I need discussion to figure out.
> You had direct, repeated, feedback on that already by a maintainer and
> one of the most experienced dev and reviewer on bridges. If you need
> more guidance, you can definitely ask questions, but asking questions
> and telling them they are wrong is very different.
>
>>> We won't tolerate insulting comments though.
>> There is *no* insulting, please don't misunderstanding before
>> *sufficient* communication, OK? Originally, I thought Dmitry may
>> ignore(or overlook) what is the current status.
> Saying to someone maintaining and/or reviewing that code for years now
> that they are wrong and should go read the code is insulting.

OK, I will remind my written words in the future.
I will back to investigate for a period of time.
Thanks a lot for reviewing.


> Maxime

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

On 2023/11/24 16:13, Maxime Ripard wrote:
> On Fri, Nov 24, 2023 at 03:51:00PM +0800, Sui Jingfeng wrote:
>> Hi,
>>
>> On 2023/11/24 15:38, Maxime Ripard wrote:
>>> On Fri, Nov 24, 2023 at 01:52:26AM +0800, Sui Jingfeng wrote:
>>>> On 2023/11/23 16:08, Dmitry Baryshkov wrote:
>>>>>> I'm agree with the idea that drm bridges drivers involved toward to a direction
>>>>>> that support more complex design, but I think we should also leave a way for the
>>>>>> most frequent use case. Make it straight-forward as a canonical design.
>>>>> Not having anything connector-related in the drm_bridge driver is a
>>>>> canonical design.
>>>> What you said is just for the more complex uses case. I can't agree, sorry.
>>>>
>>>> By choosing the word "canonical design", I means that the most frequently used
>>>> cases in practice are the canonical design, 95+% motherboards I have seen has
>>>> only one *onboard* display bridges chip. For my driver, I abstract the internal
>>>> (inside of the chip) encoder as drm_encoder and abstract the external TX chip as
>>>> drm_bridge, this design still works very well.
>>>>
>>>>
>>>> Originally, I means that this is a concept of the hardware design.
>>>> You are wrong even through in the software design context, the
>>>> transparent simple drm bridge drivers(simple-bridge.c) also *allow*
>>>> to create drm connector manually. I don't think I need to emulate
>>>> more example, please read the code by youself.
>> 'emulate' -> 'enumerate'
>>
>>> Ok. That's it. We've been patient long enough. You have been given a
>>> review and a list of things to fix for your driver to be merged.
>> This series is not relevant to my driver, can we please *limit* the
>> discussion to this series?
> Right, I conflated the two, I meant this series, or the general goal to
> enable that bridge with your driver. The rest of the driver is of course
> unaffected.
>
>>> Whether you follow them or not is your decision.
>> I'm not saying that I will not follow, just to make sure what's
>> solution is you want. I need discussion to figure out.
> You had direct, repeated, feedback on that already by a maintainer and
> one of the most experienced dev and reviewer on bridges. If you need
> more guidance, you can definitely ask questions, but asking questions
> and telling them they are wrong is very different.
>
>>> We won't tolerate insulting comments though.
>> There is *no* insulting, please don't misunderstanding before
>> *sufficient* communication, OK? Originally, I thought Dmitry may
>> ignore(or overlook) what is the current status.
> Saying to someone maintaining and/or reviewing that code for years now
> that they are wrong and should go read the code is insulting.


Back to that time, I'm focus on kindly technique debating, this is a kind
of defensive reply for the patch. This is a kind of remind in case of ignores.
Probably a bit of negative, but please don't misunderstanding as insult anyway.

Dmitry, really thanks a lot for the instructs, I have learned a lot while
talking with you. I will back to try mentioned method.

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,


On 2023/11/15 00:30, Dmitry Baryshkov wrote:
>> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
>>                                   enum drm_bridge_attach_flags flags)
>>   {
>>          struct it66121_ctx *ctx = bridge_to_it66121(bridge);
>> +       struct drm_bridge *next_bridge = ctx->next_bridge;
>> +       struct drm_encoder *encoder = bridge->encoder;
>>          int ret;
>>
>> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>> -               return -EINVAL;
>> +       if (next_bridge) {
>> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
>> +                       WARN_ON(1);
> Why? At least use WARN() instead

If (next_bridge) is true, it says that the driver *already* known that
it66121 have a next bridges attached. Then it66121 driver should certainly
attach it, no matter what it is. Either a connector or another display bridge.
It also says that this is a DT-based system on such a case. CallingWARN_ON(1) here helps to see(print) which DC driver is doing the wired 
things. Ok, I will remove the WARN_ON(1)  on the next version.

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Dmitry Baryshkov]

On Thu, 16 Nov 2023 at 12:29, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Hi,
>
>
> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
> >> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
> >>                                   enum drm_bridge_attach_flags flags)
> >>   {
> >>          struct it66121_ctx *ctx = bridge_to_it66121(bridge);
> >> +       struct drm_bridge *next_bridge = ctx->next_bridge;
> >> +       struct drm_encoder *encoder = bridge->encoder;
> >>          int ret;
> >>
> >> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> >> -               return -EINVAL;
> >> +       if (next_bridge) {
> >> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> >> +                       WARN_ON(1);
> > Why? At least use WARN() instead
>
> If (next_bridge) is true, it says that the driver *already* known that
> it66121 have a next bridges attached. Then it66121 driver should certainly
> attach it, no matter what it is. Either a connector or another display bridge.
> It also says that this is a DT-based system on such a case. CallingWARN_ON(1) here helps to see(print) which DC driver is doing the wired
> things. Ok, I will remove the WARN_ON(1)  on the next version.

That's why I pointed you to WARN(). WARN_ON(1) gives no information to
the user. WARN() allows you to add a message.

-- 
With best wishes
Dmitry

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Jani Nikula]

On Thu, 16 Nov 2023, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> On Thu, 16 Nov 2023 at 12:29, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>
>> Hi,
>>
>>
>> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
>> >> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
>> >>                                   enum drm_bridge_attach_flags flags)
>> >>   {
>> >>          struct it66121_ctx *ctx = bridge_to_it66121(bridge);
>> >> +       struct drm_bridge *next_bridge = ctx->next_bridge;
>> >> +       struct drm_encoder *encoder = bridge->encoder;
>> >>          int ret;
>> >>
>> >> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>> >> -               return -EINVAL;
>> >> +       if (next_bridge) {
>> >> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
>> >> +                       WARN_ON(1);
>> > Why? At least use WARN() instead
>>
>> If (next_bridge) is true, it says that the driver *already* known that
>> it66121 have a next bridges attached. Then it66121 driver should certainly
>> attach it, no matter what it is. Either a connector or another display bridge.
>> It also says that this is a DT-based system on such a case. CallingWARN_ON(1) here helps to see(print) which DC driver is doing the wired
>> things. Ok, I will remove the WARN_ON(1)  on the next version.
>
> That's why I pointed you to WARN(). WARN_ON(1) gives no information to
> the user. WARN() allows you to add a message.

Please use drm_WARN* while at it.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,


On 2023/11/15 00:30, Dmitry Baryshkov wrote:
>> +
>> +               ctx->connector = connector;
>> +       }
>>
>>          if (ctx->info->id == ID_IT66121) {
>>                  ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
>> @@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
>>          "vcn33", "vcn18", "vrf12"
>>   };
>>
>> -static int it66121_probe(struct i2c_client *client)
>> +int it66121_create_bridge(struct i2c_client *client, bool of_support,
>> +                         bool hpd_support, bool audio_support,
>> +                         struct drm_bridge **bridge)
>>   {
>> +       struct device *dev = &client->dev;
>>          int ret;
>>          struct it66121_ctx *ctx;
>> -       struct device *dev = &client->dev;
>> -
>> -       if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> -               dev_err(dev, "I2C check functionality failed.\n");
>> -               return -ENXIO;
>> -       }
>>
>>          ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>>          if (!ctx)
>> @@ -1649,24 +1665,19 @@ static int it66121_probe(struct i2c_client *client)
>>
>>          ctx->dev = dev;
>>          ctx->client = client;
>> -       ctx->info = i2c_get_match_data(client);
>> -
>> -       ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
>> -       if (ret)
>> -               return ret;
>> -
>> -       ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
>> -       if (ret)
>> -               return ret;
>> -
>> -       i2c_set_clientdata(client, ctx);
>>          mutex_init(&ctx->lock);
>>
>> -       ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it66121_supplies),
>> -                                            it66121_supplies);
>> -       if (ret) {
>> -               dev_err(dev, "Failed to enable power supplies\n");
>> -               return ret;
>> +       if (of_support) {
>> +               ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
>> +               if (ret)
>> +                       return ret;
>> +       } else {
>> +               ctx->bus_width = 24;
>> +               ctx->next_bridge = NULL;
>>          }
> A better alternative would be to turn OF calls into fwnode calls and
> to populate the fwnode properties. See
> drivers/platform/x86/intel/chtwc_int33fe.c for example.


Honestly, I don't want to leave any scratch(breadcrumbs).
I'm worries about that turn OF calls into fwnode calls will leave something unwanted.

Because I am not sure if fwnode calls will make sense in the DT world, while my patch
*still* be useful in the DT world. Because the newly introduced it66121_create_bridge()
function is a core. I think It's better leave this task to a more advance programmer.
if there have use case. It can be introduced at a latter time, probably parallel with
the DT.

I think DT and/or ACPI is best for integrated devices, but it66121 display bridges is
a i2c slave device. Personally, I think slave device shouldn't be standalone. I'm more
prefer to turn this driver to support hot-plug, even remove the device on the run time
freely when detach and allow reattach. Like the I2C EEPROM device in the monitor (which
contains the EDID, with I2C slave address 0x50). The I2C EEPROM device *also* don't has
a corresponding struct device representation in linux kernel.

so I still think It is best to make this drivers functional as a static lib, but I want
to hear you to say more. Why it would be a *better* alternative to turn OF calls into
fwnode calls? what are the potential benefits?

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Dmitry Baryshkov]

On Thu, 16 Nov 2023 at 13:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Hi,
>
>
> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
> >> +
> >> +               ctx->connector = connector;
> >> +       }
> >>
> >>          if (ctx->info->id == ID_IT66121) {
> >>                  ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
> >> @@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
> >>          "vcn33", "vcn18", "vrf12"
> >>   };
> >>
> >> -static int it66121_probe(struct i2c_client *client)
> >> +int it66121_create_bridge(struct i2c_client *client, bool of_support,
> >> +                         bool hpd_support, bool audio_support,
> >> +                         struct drm_bridge **bridge)
> >>   {
> >> +       struct device *dev = &client->dev;
> >>          int ret;
> >>          struct it66121_ctx *ctx;
> >> -       struct device *dev = &client->dev;
> >> -
> >> -       if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> >> -               dev_err(dev, "I2C check functionality failed.\n");
> >> -               return -ENXIO;
> >> -       }
> >>
> >>          ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> >>          if (!ctx)
> >> @@ -1649,24 +1665,19 @@ static int it66121_probe(struct i2c_client *client)
> >>
> >>          ctx->dev = dev;
> >>          ctx->client = client;
> >> -       ctx->info = i2c_get_match_data(client);
> >> -
> >> -       ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
> >> -       if (ret)
> >> -               return ret;
> >> -
> >> -       ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
> >> -       if (ret)
> >> -               return ret;
> >> -
> >> -       i2c_set_clientdata(client, ctx);
> >>          mutex_init(&ctx->lock);
> >>
> >> -       ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it66121_supplies),
> >> -                                            it66121_supplies);
> >> -       if (ret) {
> >> -               dev_err(dev, "Failed to enable power supplies\n");
> >> -               return ret;
> >> +       if (of_support) {
> >> +               ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
> >> +               if (ret)
> >> +                       return ret;
> >> +
> >> +               ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
> >> +               if (ret)
> >> +                       return ret;
> >> +       } else {
> >> +               ctx->bus_width = 24;
> >> +               ctx->next_bridge = NULL;
> >>          }
> > A better alternative would be to turn OF calls into fwnode calls and
> > to populate the fwnode properties. See
> > drivers/platform/x86/intel/chtwc_int33fe.c for example.
>
>
> Honestly, I don't want to leave any scratch(breadcrumbs).
> I'm worries about that turn OF calls into fwnode calls will leave something unwanted.
>
> Because I am not sure if fwnode calls will make sense in the DT world, while my patch
> *still* be useful in the DT world.

fwnode calls work for both DT and non-DT cases. In the DT case they
work with DT nodes and properties. In the non-DT case, they work with
manually populated properties.

> Because the newly introduced it66121_create_bridge()
> function is a core. I think It's better leave this task to a more advance programmer.
> if there have use case. It can be introduced at a latter time, probably parallel with
> the DT.
>
> I think DT and/or ACPI is best for integrated devices, but it66121 display bridges is
> a i2c slave device. Personally, I think slave device shouldn't be standalone. I'm more
> prefer to turn this driver to support hot-plug, even remove the device on the run time
> freely when detach and allow reattach. Like the I2C EEPROM device in the monitor (which
> contains the EDID, with I2C slave address 0x50). The I2C EEPROM device *also* don't has
> a corresponding struct device representation in linux kernel.

It has. See i2c_client::dev.

> so I still think It is best to make this drivers functional as a static lib, but I want
> to hear you to say more. Why it would be a *better* alternative to turn OF calls into
> fwnode calls? what are the potential benefits?

Because then you can populate device properties from your root device.
Because it allows the platform to specify the bus width instead of
hardcoding 24 bits (which might work in your case, but might not be
applicable to another user next week).

Anyway, even without fwnode, I'd strongly suggest you to drop the
it66121_create_bridge() as it is now and start by populating the i2c
bus from your root device. Then you will need some way (fwnode?) to
discover the bridge chain. And at the last point you will get into the
device data and/or properties business.

-- 
With best wishes
Dmitry

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,


On 2023/11/16 19:29, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 13:18, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>> Hi,
>>
>>
>> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
>>>> +
>>>> +               ctx->connector = connector;
>>>> +       }
>>>>
>>>>           if (ctx->info->id == ID_IT66121) {
>>>>                   ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
>>>> @@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
>>>>           "vcn33", "vcn18", "vrf12"
>>>>    };
>>>>
>>>> -static int it66121_probe(struct i2c_client *client)
>>>> +int it66121_create_bridge(struct i2c_client *client, bool of_support,
>>>> +                         bool hpd_support, bool audio_support,
>>>> +                         struct drm_bridge **bridge)
>>>>    {
>>>> +       struct device *dev = &client->dev;
>>>>           int ret;
>>>>           struct it66121_ctx *ctx;
>>>> -       struct device *dev = &client->dev;
>>>> -
>>>> -       if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>>>> -               dev_err(dev, "I2C check functionality failed.\n");
>>>> -               return -ENXIO;
>>>> -       }
>>>>
>>>>           ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>>>>           if (!ctx)
>>>> @@ -1649,24 +1665,19 @@ static int it66121_probe(struct i2c_client *client)
>>>>
>>>>           ctx->dev = dev;
>>>>           ctx->client = client;
>>>> -       ctx->info = i2c_get_match_data(client);
>>>> -
>>>> -       ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
>>>> -       if (ret)
>>>> -               return ret;
>>>> -
>>>> -       ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
>>>> -       if (ret)
>>>> -               return ret;
>>>> -
>>>> -       i2c_set_clientdata(client, ctx);
>>>>           mutex_init(&ctx->lock);
>>>>
>>>> -       ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it66121_supplies),
>>>> -                                            it66121_supplies);
>>>> -       if (ret) {
>>>> -               dev_err(dev, "Failed to enable power supplies\n");
>>>> -               return ret;
>>>> +       if (of_support) {
>>>> +               ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
>>>> +               if (ret)
>>>> +                       return ret;
>>>> +
>>>> +               ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
>>>> +               if (ret)
>>>> +                       return ret;
>>>> +       } else {
>>>> +               ctx->bus_width = 24;
>>>> +               ctx->next_bridge = NULL;
>>>>           }
>>> A better alternative would be to turn OF calls into fwnode calls and
>>> to populate the fwnode properties. See
>>> drivers/platform/x86/intel/chtwc_int33fe.c for example.
>>
>> Honestly, I don't want to leave any scratch(breadcrumbs).
>> I'm worries about that turn OF calls into fwnode calls will leave something unwanted.
>>
>> Because I am not sure if fwnode calls will make sense in the DT world, while my patch
>> *still* be useful in the DT world.
> fwnode calls work for both DT and non-DT cases. In the DT case they
> work with DT nodes and properties. In the non-DT case, they work with
> manually populated properties.
>
>> Because the newly introduced it66121_create_bridge()
>> function is a core. I think It's better leave this task to a more advance programmer.
>> if there have use case. It can be introduced at a latter time, probably parallel with
>> the DT.
>>
>> I think DT and/or ACPI is best for integrated devices, but it66121 display bridges is
>> a i2c slave device. Personally, I think slave device shouldn't be standalone. I'm more
>> prefer to turn this driver to support hot-plug, even remove the device on the run time
>> freely when detach and allow reattach. Like the I2C EEPROM device in the monitor (which
>> contains the EDID, with I2C slave address 0x50). The I2C EEPROM device *also* don't has
>> a corresponding struct device representation in linux kernel.
> It has. See i2c_client::dev.

No, what I mean is that there don't have a device driver for monitor(display) hardware entity.
And the drm_do_probe_ddc_edid() is the static linked driver, which is similar with the idea
this series want to express.


>> so I still think It is best to make this drivers functional as a static lib, but I want
>> to hear you to say more. Why it would be a *better* alternative to turn OF calls into
>> fwnode calls? what are the potential benefits?
> Because then you can populate device properties from your root device.
> Because it allows the platform to specify the bus width instead of
> hardcoding 24 bits (which might work in your case, but might not be
> applicable to another user next week).


No, this problem can be easily solved. Simply add another argument.

```

int it66121_create_bridge(struct i2c_client *client, bool of_support,
                           bool hpd_support, bool audio_support, u32 bus_width,
                           struct drm_bridge **bridge);
```


> Anyway, even without fwnode, I'd strongly suggest you to drop the
> it66121_create_bridge() as it is now and start by populating the i2c
> bus from your root device.

This will force all non-DT users to add the similar code patter at the display controller side,
which is another kind of duplication. The monitor is also as I2C slave device, can be abstract
as a identify drm bridges in theory, I guess.


> Then you will need some way (fwnode?) to
> discover the bridge chain. And at the last point you will get into the
> device data and/or properties business.
>
No, leave that chance to a more better programmer and forgive me please,
too difficult, I'm afraid of not able to solve. Thanks a lot for the trust!

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

On 2023/11/16 19:53, Sui Jingfeng wrote:
> Hi,
>
>
> On 2023/11/16 19:29, Dmitry Baryshkov wrote:
>> On Thu, 16 Nov 2023 at 13:18, Sui Jingfeng <sui.jingfeng@linux.dev> 
>> wrote:
>>> Hi,
>>>
>>>
>>> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
>>>>> +
>>>>> +               ctx->connector = connector;
>>>>> +       }
>>>>>
>>>>>           if (ctx->info->id == ID_IT66121) {
>>>>>                   ret = regmap_write_bits(ctx->regmap, 
>>>>> IT66121_CLK_BANK_REG,
>>>>> @@ -1632,16 +1651,13 @@ static const char * const 
>>>>> it66121_supplies[] = {
>>>>>           "vcn33", "vcn18", "vrf12"
>>>>>    };
>>>>>
>>>>> -static int it66121_probe(struct i2c_client *client)
>>>>> +int it66121_create_bridge(struct i2c_client *client, bool 
>>>>> of_support,
>>>>> +                         bool hpd_support, bool audio_support,
>>>>> +                         struct drm_bridge **bridge)
>>>>>    {
>>>>> +       struct device *dev = &client->dev;
>>>>>           int ret;
>>>>>           struct it66121_ctx *ctx;
>>>>> -       struct device *dev = &client->dev;
>>>>> -
>>>>> -       if (!i2c_check_functionality(client->adapter, 
>>>>> I2C_FUNC_I2C)) {
>>>>> -               dev_err(dev, "I2C check functionality failed.\n");
>>>>> -               return -ENXIO;
>>>>> -       }
>>>>>
>>>>>           ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>>>>>           if (!ctx)
>>>>> @@ -1649,24 +1665,19 @@ static int it66121_probe(struct i2c_client 
>>>>> *client)
>>>>>
>>>>>           ctx->dev = dev;
>>>>>           ctx->client = client;
>>>>> -       ctx->info = i2c_get_match_data(client);
>>>>> -
>>>>> -       ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
>>>>> -       if (ret)
>>>>> -               return ret;
>>>>> -
>>>>> -       ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
>>>>> -       if (ret)
>>>>> -               return ret;
>>>>> -
>>>>> -       i2c_set_clientdata(client, ctx);
>>>>>           mutex_init(&ctx->lock);
>>>>>
>>>>> -       ret = devm_regulator_bulk_get_enable(dev, 
>>>>> ARRAY_SIZE(it66121_supplies),
>>>>> - it66121_supplies);
>>>>> -       if (ret) {
>>>>> -               dev_err(dev, "Failed to enable power supplies\n");
>>>>> -               return ret;
>>>>> +       if (of_support) {
>>>>> +               ret = it66121_of_read_bus_width(dev, 
>>>>> &ctx->bus_width);
>>>>> +               if (ret)
>>>>> +                       return ret;
>>>>> +
>>>>> +               ret = it66121_of_get_next_bridge(dev, 
>>>>> &ctx->next_bridge);
>>>>> +               if (ret)
>>>>> +                       return ret;
>>>>> +       } else {
>>>>> +               ctx->bus_width = 24;
>>>>> +               ctx->next_bridge = NULL;
>>>>>           }
>>>> A better alternative would be to turn OF calls into fwnode calls and
>>>> to populate the fwnode properties. See
>>>> drivers/platform/x86/intel/chtwc_int33fe.c for example.
>>>
>>> Honestly, I don't want to leave any scratch(breadcrumbs).
>>> I'm worries about that turn OF calls into fwnode calls will leave 
>>> something unwanted.
>>>
>>> Because I am not sure if fwnode calls will make sense in the DT 
>>> world, while my patch
>>> *still* be useful in the DT world.
>> fwnode calls work for both DT and non-DT cases. In the DT case they
>> work with DT nodes and properties. In the non-DT case, they work with
>> manually populated properties.
>>
>>> Because the newly introduced it66121_create_bridge()
>>> function is a core. I think It's better leave this task to a more 
>>> advance programmer.
>>> if there have use case. It can be introduced at a latter time, 
>>> probably parallel with
>>> the DT.
>>>
>>> I think DT and/or ACPI is best for integrated devices, but it66121 
>>> display bridges is
>>> a i2c slave device. Personally, I think slave device shouldn't be 
>>> standalone. I'm more
>>> prefer to turn this driver to support hot-plug, even remove the 
>>> device on the run time
>>> freely when detach and allow reattach. Like the I2C EEPROM device in 
>>> the monitor (which
>>> contains the EDID, with I2C slave address 0x50). The I2C EEPROM 
>>> device *also* don't has
>>> a corresponding struct device representation in linux kernel.
>> It has. See i2c_client::dev.
>
> No, what I mean is that there don't have a device driver for 
> monitor(display) hardware entity.
> And the drm_do_probe_ddc_edid() is the static linked driver, which is 
> similar with the idea
> this series want to express.
>
>
>>> so I still think It is best to make this drivers functional as a 
>>> static lib, but I want
>>> to hear you to say more. Why it would be a *better* alternative to 
>>> turn OF calls into
>>> fwnode calls? what are the potential benefits?
>> Because then you can populate device properties from your root device.
>> Because it allows the platform to specify the bus width instead of
>> hardcoding 24 bits (which might work in your case, but might not be
>> applicable to another user next week).
>
>
> No, this problem can be easily solved. Simply add another argument.
>
> ```
>
> int it66121_create_bridge(struct i2c_client *client, bool of_support,
>                           bool hpd_support, bool audio_support, u32 
> bus_width,
>                           struct drm_bridge **bridge);
> ```
>
>
>> Anyway, even without fwnode, I'd strongly suggest you to drop the
>> it66121_create_bridge() as it is now and start by populating the i2c
>> bus from your root device.
>
> This will force all non-DT users to add the similar code patter at the 
> display controller side,
> which is another kind of duplication. The monitor is also as I2C slave 
> device, can be abstract
> as a identify drm bridges in theory, I guess.
>

'identify' -> 'identity'


>
>> Then you will need some way (fwnode?) to
>> discover the bridge chain. And at the last point you will get into the
>> device data and/or properties business.
>>
> No, leave that chance to a more better programmer and forgive me please,
> too difficult, I'm afraid of not able to solve. Thanks a lot for the 
> trust!
>
>

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Dmitry Baryshkov]

On Thu, 16 Nov 2023 at 14:08, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
>
> On 2023/11/16 19:53, Sui Jingfeng wrote:
> > Hi,
> >
> >
> > On 2023/11/16 19:29, Dmitry Baryshkov wrote:
> >> On Thu, 16 Nov 2023 at 13:18, Sui Jingfeng <sui.jingfeng@linux.dev>
> >> wrote:
> >>> Hi,
> >>>
> >>>
> >>> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
> >>>>> +
> >>>>> +               ctx->connector = connector;
> >>>>> +       }
> >>>>>
> >>>>>           if (ctx->info->id == ID_IT66121) {
> >>>>>                   ret = regmap_write_bits(ctx->regmap,
> >>>>> IT66121_CLK_BANK_REG,
> >>>>> @@ -1632,16 +1651,13 @@ static const char * const
> >>>>> it66121_supplies[] = {
> >>>>>           "vcn33", "vcn18", "vrf12"
> >>>>>    };
> >>>>>
> >>>>> -static int it66121_probe(struct i2c_client *client)
> >>>>> +int it66121_create_bridge(struct i2c_client *client, bool
> >>>>> of_support,
> >>>>> +                         bool hpd_support, bool audio_support,
> >>>>> +                         struct drm_bridge **bridge)
> >>>>>    {
> >>>>> +       struct device *dev = &client->dev;
> >>>>>           int ret;
> >>>>>           struct it66121_ctx *ctx;
> >>>>> -       struct device *dev = &client->dev;
> >>>>> -
> >>>>> -       if (!i2c_check_functionality(client->adapter,
> >>>>> I2C_FUNC_I2C)) {
> >>>>> -               dev_err(dev, "I2C check functionality failed.\n");
> >>>>> -               return -ENXIO;
> >>>>> -       }
> >>>>>
> >>>>>           ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> >>>>>           if (!ctx)
> >>>>> @@ -1649,24 +1665,19 @@ static int it66121_probe(struct i2c_client
> >>>>> *client)
> >>>>>
> >>>>>           ctx->dev = dev;
> >>>>>           ctx->client = client;
> >>>>> -       ctx->info = i2c_get_match_data(client);
> >>>>> -
> >>>>> -       ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
> >>>>> -       if (ret)
> >>>>> -               return ret;
> >>>>> -
> >>>>> -       ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
> >>>>> -       if (ret)
> >>>>> -               return ret;
> >>>>> -
> >>>>> -       i2c_set_clientdata(client, ctx);
> >>>>>           mutex_init(&ctx->lock);
> >>>>>
> >>>>> -       ret = devm_regulator_bulk_get_enable(dev,
> >>>>> ARRAY_SIZE(it66121_supplies),
> >>>>> - it66121_supplies);
> >>>>> -       if (ret) {
> >>>>> -               dev_err(dev, "Failed to enable power supplies\n");
> >>>>> -               return ret;
> >>>>> +       if (of_support) {
> >>>>> +               ret = it66121_of_read_bus_width(dev,
> >>>>> &ctx->bus_width);
> >>>>> +               if (ret)
> >>>>> +                       return ret;
> >>>>> +
> >>>>> +               ret = it66121_of_get_next_bridge(dev,
> >>>>> &ctx->next_bridge);
> >>>>> +               if (ret)
> >>>>> +                       return ret;
> >>>>> +       } else {
> >>>>> +               ctx->bus_width = 24;
> >>>>> +               ctx->next_bridge = NULL;
> >>>>>           }
> >>>> A better alternative would be to turn OF calls into fwnode calls and
> >>>> to populate the fwnode properties. See
> >>>> drivers/platform/x86/intel/chtwc_int33fe.c for example.
> >>>
> >>> Honestly, I don't want to leave any scratch(breadcrumbs).
> >>> I'm worries about that turn OF calls into fwnode calls will leave
> >>> something unwanted.
> >>>
> >>> Because I am not sure if fwnode calls will make sense in the DT
> >>> world, while my patch
> >>> *still* be useful in the DT world.
> >> fwnode calls work for both DT and non-DT cases. In the DT case they
> >> work with DT nodes and properties. In the non-DT case, they work with
> >> manually populated properties.
> >>
> >>> Because the newly introduced it66121_create_bridge()
> >>> function is a core. I think It's better leave this task to a more
> >>> advance programmer.
> >>> if there have use case. It can be introduced at a latter time,
> >>> probably parallel with
> >>> the DT.
> >>>
> >>> I think DT and/or ACPI is best for integrated devices, but it66121
> >>> display bridges is
> >>> a i2c slave device. Personally, I think slave device shouldn't be
> >>> standalone. I'm more
> >>> prefer to turn this driver to support hot-plug, even remove the
> >>> device on the run time
> >>> freely when detach and allow reattach. Like the I2C EEPROM device in
> >>> the monitor (which
> >>> contains the EDID, with I2C slave address 0x50). The I2C EEPROM
> >>> device *also* don't has
> >>> a corresponding struct device representation in linux kernel.
> >> It has. See i2c_client::dev.
> >
> > No, what I mean is that there don't have a device driver for
> > monitor(display) hardware entity.
> > And the drm_do_probe_ddc_edid() is the static linked driver, which is
> > similar with the idea
> > this series want to express.

Because the monitor is not a part of the display pipeline.

> >
> >
> >>> so I still think It is best to make this drivers functional as a
> >>> static lib, but I want
> >>> to hear you to say more. Why it would be a *better* alternative to
> >>> turn OF calls into
> >>> fwnode calls? what are the potential benefits?
> >> Because then you can populate device properties from your root device.
> >> Because it allows the platform to specify the bus width instead of
> >> hardcoding 24 bits (which might work in your case, but might not be
> >> applicable to another user next week).
> >
> >
> > No, this problem can be easily solved. Simply add another argument.
> >
> > ```
> >
> > int it66121_create_bridge(struct i2c_client *client, bool of_support,
> >                           bool hpd_support, bool audio_support, u32
> > bus_width,
> >                           struct drm_bridge **bridge);
> > ```
> >
> >
> >> Anyway, even without fwnode, I'd strongly suggest you to drop the
> >> it66121_create_bridge() as it is now and start by populating the i2c
> >> bus from your root device.
> >
> > This will force all non-DT users to add the similar code patter at the
> > display controller side,
> > which is another kind of duplication. The monitor is also as I2C slave
> > device, can be abstract
> > as a identify drm bridges in theory, I guess.
> >
>
> 'identify' -> 'identity'
>
>
> >
> >> Then you will need some way (fwnode?) to
> >> discover the bridge chain. And at the last point you will get into the
> >> device data and/or properties business.
> >>
> > No, leave that chance to a more better programmer and forgive me please,
> > too difficult, I'm afraid of not able to solve. Thanks a lot for the
> > trust!

From my point of view: no.

-- 
With best wishes
Dmitry

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

On 2023/11/16 23:23, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 14:08, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>
>> On 2023/11/16 19:53, Sui Jingfeng wrote:
>>> Hi,
>>>
>>>
>>> On 2023/11/16 19:29, Dmitry Baryshkov wrote:
>>>> On Thu, 16 Nov 2023 at 13:18, Sui Jingfeng <sui.jingfeng@linux.dev>
>>>> wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
>>>>>>> +
>>>>>>> +               ctx->connector = connector;
>>>>>>> +       }
>>>>>>>
>>>>>>>            if (ctx->info->id == ID_IT66121) {
>>>>>>>                    ret = regmap_write_bits(ctx->regmap,
>>>>>>> IT66121_CLK_BANK_REG,
>>>>>>> @@ -1632,16 +1651,13 @@ static const char * const
>>>>>>> it66121_supplies[] = {
>>>>>>>            "vcn33", "vcn18", "vrf12"
>>>>>>>     };
>>>>>>>
>>>>>>> -static int it66121_probe(struct i2c_client *client)
>>>>>>> +int it66121_create_bridge(struct i2c_client *client, bool
>>>>>>> of_support,
>>>>>>> +                         bool hpd_support, bool audio_support,
>>>>>>> +                         struct drm_bridge **bridge)
>>>>>>>     {
>>>>>>> +       struct device *dev = &client->dev;
>>>>>>>            int ret;
>>>>>>>            struct it66121_ctx *ctx;
>>>>>>> -       struct device *dev = &client->dev;
>>>>>>> -
>>>>>>> -       if (!i2c_check_functionality(client->adapter,
>>>>>>> I2C_FUNC_I2C)) {
>>>>>>> -               dev_err(dev, "I2C check functionality failed.\n");
>>>>>>> -               return -ENXIO;
>>>>>>> -       }
>>>>>>>
>>>>>>>            ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>>>>>>>            if (!ctx)
>>>>>>> @@ -1649,24 +1665,19 @@ static int it66121_probe(struct i2c_client
>>>>>>> *client)
>>>>>>>
>>>>>>>            ctx->dev = dev;
>>>>>>>            ctx->client = client;
>>>>>>> -       ctx->info = i2c_get_match_data(client);
>>>>>>> -
>>>>>>> -       ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
>>>>>>> -       if (ret)
>>>>>>> -               return ret;
>>>>>>> -
>>>>>>> -       ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
>>>>>>> -       if (ret)
>>>>>>> -               return ret;
>>>>>>> -
>>>>>>> -       i2c_set_clientdata(client, ctx);
>>>>>>>            mutex_init(&ctx->lock);
>>>>>>>
>>>>>>> -       ret = devm_regulator_bulk_get_enable(dev,
>>>>>>> ARRAY_SIZE(it66121_supplies),
>>>>>>> - it66121_supplies);
>>>>>>> -       if (ret) {
>>>>>>> -               dev_err(dev, "Failed to enable power supplies\n");
>>>>>>> -               return ret;
>>>>>>> +       if (of_support) {
>>>>>>> +               ret = it66121_of_read_bus_width(dev,
>>>>>>> &ctx->bus_width);
>>>>>>> +               if (ret)
>>>>>>> +                       return ret;
>>>>>>> +
>>>>>>> +               ret = it66121_of_get_next_bridge(dev,
>>>>>>> &ctx->next_bridge);
>>>>>>> +               if (ret)
>>>>>>> +                       return ret;
>>>>>>> +       } else {
>>>>>>> +               ctx->bus_width = 24;
>>>>>>> +               ctx->next_bridge = NULL;
>>>>>>>            }
>>>>>> A better alternative would be to turn OF calls into fwnode calls and
>>>>>> to populate the fwnode properties. See
>>>>>> drivers/platform/x86/intel/chtwc_int33fe.c for example.
>>>>> Honestly, I don't want to leave any scratch(breadcrumbs).
>>>>> I'm worries about that turn OF calls into fwnode calls will leave
>>>>> something unwanted.
>>>>>
>>>>> Because I am not sure if fwnode calls will make sense in the DT
>>>>> world, while my patch
>>>>> *still* be useful in the DT world.
>>>> fwnode calls work for both DT and non-DT cases. In the DT case they
>>>> work with DT nodes and properties. In the non-DT case, they work with
>>>> manually populated properties.
>>>>
>>>>> Because the newly introduced it66121_create_bridge()
>>>>> function is a core. I think It's better leave this task to a more
>>>>> advance programmer.
>>>>> if there have use case. It can be introduced at a latter time,
>>>>> probably parallel with
>>>>> the DT.
>>>>>
>>>>> I think DT and/or ACPI is best for integrated devices, but it66121
>>>>> display bridges is
>>>>> a i2c slave device. Personally, I think slave device shouldn't be
>>>>> standalone. I'm more
>>>>> prefer to turn this driver to support hot-plug, even remove the
>>>>> device on the run time
>>>>> freely when detach and allow reattach. Like the I2C EEPROM device in
>>>>> the monitor (which
>>>>> contains the EDID, with I2C slave address 0x50). The I2C EEPROM
>>>>> device *also* don't has
>>>>> a corresponding struct device representation in linux kernel.
>>>> It has. See i2c_client::dev.
>>> No, what I mean is that there don't have a device driver for
>>> monitor(display) hardware entity.
>>> And the drm_do_probe_ddc_edid() is the static linked driver, which is
>>> similar with the idea
>>> this series want to express.
> Because the monitor is not a part of the display pipeline.
>
I think the monitor *is definitely* part of the display pipeline, and it
is the most important part of the entire display pipeline.

1)

DPMS, self-refreshing, display timings, resolutions supported, HDR, DSC,
gsync and freesync etc can be part of whole mode-set. Please consider
what the various ->mode_valid() and -> the atomic_check() are for?

2)

If the monitor is not a part of the display pipeline, then the various
display panels hardware should also not be part of the display pipeline.
Because they are all belong to display category.
  
the monitor = panel + panel drive IC(such as RTD2281CL, HT1622, ssd130x).

There are panel bridges which abstract the panel + connector as a drm bridge,
why the bare panel can be part of the display pipeline, while the more complex
monitor can't be?

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6886 bytes --]

On Fri, Nov 17, 2023 at 01:18:49AM +0800, Sui Jingfeng wrote:
> 
> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
> > On Thu, 16 Nov 2023 at 14:08, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> > > 
> > > On 2023/11/16 19:53, Sui Jingfeng wrote:
> > > > Hi,
> > > > 
> > > > 
> > > > On 2023/11/16 19:29, Dmitry Baryshkov wrote:
> > > > > On Thu, 16 Nov 2023 at 13:18, Sui Jingfeng <sui.jingfeng@linux.dev>
> > > > > wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > 
> > > > > > On 2023/11/15 00:30, Dmitry Baryshkov wrote:
> > > > > > > > +
> > > > > > > > +               ctx->connector = connector;
> > > > > > > > +       }
> > > > > > > > 
> > > > > > > >            if (ctx->info->id == ID_IT66121) {
> > > > > > > >                    ret = regmap_write_bits(ctx->regmap,
> > > > > > > > IT66121_CLK_BANK_REG,
> > > > > > > > @@ -1632,16 +1651,13 @@ static const char * const
> > > > > > > > it66121_supplies[] = {
> > > > > > > >            "vcn33", "vcn18", "vrf12"
> > > > > > > >     };
> > > > > > > > 
> > > > > > > > -static int it66121_probe(struct i2c_client *client)
> > > > > > > > +int it66121_create_bridge(struct i2c_client *client, bool
> > > > > > > > of_support,
> > > > > > > > +                         bool hpd_support, bool audio_support,
> > > > > > > > +                         struct drm_bridge **bridge)
> > > > > > > >     {
> > > > > > > > +       struct device *dev = &client->dev;
> > > > > > > >            int ret;
> > > > > > > >            struct it66121_ctx *ctx;
> > > > > > > > -       struct device *dev = &client->dev;
> > > > > > > > -
> > > > > > > > -       if (!i2c_check_functionality(client->adapter,
> > > > > > > > I2C_FUNC_I2C)) {
> > > > > > > > -               dev_err(dev, "I2C check functionality failed.\n");
> > > > > > > > -               return -ENXIO;
> > > > > > > > -       }
> > > > > > > > 
> > > > > > > >            ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > > > > > > >            if (!ctx)
> > > > > > > > @@ -1649,24 +1665,19 @@ static int it66121_probe(struct i2c_client
> > > > > > > > *client)
> > > > > > > > 
> > > > > > > >            ctx->dev = dev;
> > > > > > > >            ctx->client = client;
> > > > > > > > -       ctx->info = i2c_get_match_data(client);
> > > > > > > > -
> > > > > > > > -       ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
> > > > > > > > -       if (ret)
> > > > > > > > -               return ret;
> > > > > > > > -
> > > > > > > > -       ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
> > > > > > > > -       if (ret)
> > > > > > > > -               return ret;
> > > > > > > > -
> > > > > > > > -       i2c_set_clientdata(client, ctx);
> > > > > > > >            mutex_init(&ctx->lock);
> > > > > > > > 
> > > > > > > > -       ret = devm_regulator_bulk_get_enable(dev,
> > > > > > > > ARRAY_SIZE(it66121_supplies),
> > > > > > > > - it66121_supplies);
> > > > > > > > -       if (ret) {
> > > > > > > > -               dev_err(dev, "Failed to enable power supplies\n");
> > > > > > > > -               return ret;
> > > > > > > > +       if (of_support) {
> > > > > > > > +               ret = it66121_of_read_bus_width(dev,
> > > > > > > > &ctx->bus_width);
> > > > > > > > +               if (ret)
> > > > > > > > +                       return ret;
> > > > > > > > +
> > > > > > > > +               ret = it66121_of_get_next_bridge(dev,
> > > > > > > > &ctx->next_bridge);
> > > > > > > > +               if (ret)
> > > > > > > > +                       return ret;
> > > > > > > > +       } else {
> > > > > > > > +               ctx->bus_width = 24;
> > > > > > > > +               ctx->next_bridge = NULL;
> > > > > > > >            }
> > > > > > > A better alternative would be to turn OF calls into fwnode calls and
> > > > > > > to populate the fwnode properties. See
> > > > > > > drivers/platform/x86/intel/chtwc_int33fe.c for example.
> > > > > > Honestly, I don't want to leave any scratch(breadcrumbs).
> > > > > > I'm worries about that turn OF calls into fwnode calls will leave
> > > > > > something unwanted.
> > > > > > 
> > > > > > Because I am not sure if fwnode calls will make sense in the DT
> > > > > > world, while my patch
> > > > > > *still* be useful in the DT world.
> > > > > fwnode calls work for both DT and non-DT cases. In the DT case they
> > > > > work with DT nodes and properties. In the non-DT case, they work with
> > > > > manually populated properties.
> > > > > 
> > > > > > Because the newly introduced it66121_create_bridge()
> > > > > > function is a core. I think It's better leave this task to a more
> > > > > > advance programmer.
> > > > > > if there have use case. It can be introduced at a latter time,
> > > > > > probably parallel with
> > > > > > the DT.
> > > > > > 
> > > > > > I think DT and/or ACPI is best for integrated devices, but it66121
> > > > > > display bridges is
> > > > > > a i2c slave device. Personally, I think slave device shouldn't be
> > > > > > standalone. I'm more
> > > > > > prefer to turn this driver to support hot-plug, even remove the
> > > > > > device on the run time
> > > > > > freely when detach and allow reattach. Like the I2C EEPROM device in
> > > > > > the monitor (which
> > > > > > contains the EDID, with I2C slave address 0x50). The I2C EEPROM
> > > > > > device *also* don't has
> > > > > > a corresponding struct device representation in linux kernel.
> > > > > It has. See i2c_client::dev.
> > > > No, what I mean is that there don't have a device driver for
> > > > monitor(display) hardware entity.
> > > > And the drm_do_probe_ddc_edid() is the static linked driver, which is
> > > > similar with the idea
> > > > this series want to express.
> > Because the monitor is not a part of the display pipeline.
> > 
> I think the monitor *is definitely* part of the display pipeline, and it
> is the most important part of the entire display pipeline.
> 
> 1)
> 
> DPMS, self-refreshing, display timings, resolutions supported, HDR, DSC,
> gsync and freesync etc can be part of whole mode-set. Please consider
> what the various ->mode_valid() and -> the atomic_check() are for?
> 
> 2)
> 
> If the monitor is not a part of the display pipeline, then the various
> display panels hardware should also not be part of the display pipeline.
> Because they are all belong to display category.
> the monitor = panel + panel drive IC(such as RTD2281CL, HT1622, ssd130x).

To expand further on that, I guess one of the key difference is that you
don't really expect to interact with the EEPROM, you'll only read it,
which is fairly different from your bridge.

And if someone wanted to instatiate nvmem devices for the various
EEPROMs in the monitor, I would very much welcome that change.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,

On 2023/11/16 23:23, Dmitry Baryshkov wrote:
>>>> Then you will need some way (fwnode?) to
>>>> discover the bridge chain. And at the last point you will get into the
>>>> device data and/or properties business.
>>>>
>>> No, leave that chance to a more better programmer and forgive me please,
>>> too difficult, I'm afraid of not able to solve. Thanks a lot for the
>>> trust!
>  From my point of view: no.


I respect the fact that the community prefer generic mechanisms.
If our approach is not what the community want, can I switch back
to my previous solution? I can reduce the duplication of our
localized it66121 driver to a minimal, rewrite it until it meets
the community's requirement. I know our device looks weird and
our approach is not elegant. But at the very least, we could not
mess the community's design up by localize. Otherwise, I don't know
what is the better approach to solve such a problem.

Can I switch back or any other ideas?
  

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Dmitry Baryshkov]

On Fri, 17 Nov 2023 at 06:24, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Hi,
>
> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
> >>>> Then you will need some way (fwnode?) to
> >>>> discover the bridge chain. And at the last point you will get into the
> >>>> device data and/or properties business.
> >>>>
> >>> No, leave that chance to a more better programmer and forgive me please,
> >>> too difficult, I'm afraid of not able to solve. Thanks a lot for the
> >>> trust!
> >  From my point of view: no.
>
>
> I respect the fact that the community prefer generic mechanisms.
> If our approach is not what the community want, can I switch back
> to my previous solution? I can reduce the duplication of our
> localized it66121 driver to a minimal, rewrite it until it meets
> the community's requirement. I know our device looks weird and
> our approach is not elegant. But at the very least, we could not
> mess the community's design up by localize. Otherwise, I don't know
> what is the better approach to solve such a problem.
>
> Can I switch back or any other ideas?

I keep on repeating: create the i2c device from your root device
driver, which parses BIOS data.

-- 
With best wishes
Dmitry

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,

On 2023/11/17 17:03, Dmitry Baryshkov wrote:
> On Fri, 17 Nov 2023 at 06:24, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>> Hi,
>>
>> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
>>>>>> Then you will need some way (fwnode?) to
>>>>>> discover the bridge chain. And at the last point you will get into the
>>>>>> device data and/or properties business.
>>>>>>
>>>>> No, leave that chance to a more better programmer and forgive me please,
>>>>> too difficult, I'm afraid of not able to solve. Thanks a lot for the
>>>>> trust!
>>>   From my point of view: no.
>>
>> I respect the fact that the community prefer generic mechanisms.
>> If our approach is not what the community want, can I switch back
>> to my previous solution? I can reduce the duplication of our
>> localized it66121 driver to a minimal, rewrite it until it meets
>> the community's requirement. I know our device looks weird and
>> our approach is not elegant. But at the very least, we could not
>> mess the community's design up by localize. Otherwise, I don't know
>> what is the better approach to solve such a problem.
>>
>> Can I switch back or any other ideas?
> I keep on repeating: create the i2c device from your root device
> driver, which parses BIOS data.
>
This is not my own problems, currently it66121 (but not only) display bridge driver
don't works on X86 either. What we are trying to do is to provide a generic, non-platform
dependent solution. It is not only relevant to my driver. In fact, this series made
no assumption which hardware/display controller will be the user.

I have investigated before respin this patch, there are other hardwares which
ship the it66121 display bridge. For example, the Fresco Logic FL2000dx USB 3.0
to VGA display adapter[1][2]. Even the windows have a driver.

[1] https://github.com/FrescoLogic/FL2000
[2] https://oemdrivers.com/graphics-fresco-logic-fl2000

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Neil Armstrong]

On 17/11/2023 18:14, Sui Jingfeng wrote:
> Hi,
> 
> On 2023/11/17 17:03, Dmitry Baryshkov wrote:
>> On Fri, 17 Nov 2023 at 06:24, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>> Hi,
>>>
>>> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
>>>>>>> Then you will need some way (fwnode?) to
>>>>>>> discover the bridge chain. And at the last point you will get into the
>>>>>>> device data and/or properties business.
>>>>>>>
>>>>>> No, leave that chance to a more better programmer and forgive me please,
>>>>>> too difficult, I'm afraid of not able to solve. Thanks a lot for the
>>>>>> trust!
>>>>   From my point of view: no.
>>>
>>> I respect the fact that the community prefer generic mechanisms.
>>> If our approach is not what the community want, can I switch back
>>> to my previous solution? I can reduce the duplication of our
>>> localized it66121 driver to a minimal, rewrite it until it meets
>>> the community's requirement. I know our device looks weird and
>>> our approach is not elegant. But at the very least, we could not
>>> mess the community's design up by localize. Otherwise, I don't know
>>> what is the better approach to solve such a problem.
>>>
>>> Can I switch back or any other ideas?
>> I keep on repeating: create the i2c device from your root device
>> driver, which parses BIOS data.
>>
> This is not my own problems, currently it66121 (but not only) display bridge driver
> don't works on X86 either. What we are trying to do is to provide a generic, non-platform
> dependent solution. It is not only relevant to my driver. In fact, this series made
> no assumption which hardware/display controller will be the user.
> 
> I have investigated before respin this patch, there are other hardwares which
> ship the it66121 display bridge. For example, the Fresco Logic FL2000dx USB 3.0
> to VGA display adapter[1][2]. Even the windows have a driver.
> 
> [1] https://github.com/FrescoLogic/FL2000
> [2] https://oemdrivers.com/graphics-fresco-logic-fl2000

Switching to fwnodes, registering an i2c bus and generating fwnode data matching the
interconnect architecture is the way.

DRM Bridge transition to fwnode only should be done first, this will open bridge
to any architecture and device description (DT or ACPI).

Neil

> 

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Sui Jingfeng]

Hi,


On 2023/11/17 17:03, Dmitry Baryshkov wrote:
> On Fri, 17 Nov 2023 at 06:24, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>> Hi,
>>
>> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
>>>>>> Then you will need some way (fwnode?) to
>>>>>> discover the bridge chain. And at the last point you will get into the
>>>>>> device data and/or properties business.
>>>>>>
>>>>> No, leave that chance to a more better programmer and forgive me please,
>>>>> too difficult, I'm afraid of not able to solve. Thanks a lot for the
>>>>> trust!
>>>   From my point of view: no.
>>
>> I respect the fact that the community prefer generic mechanisms.
>> If our approach is not what the community want, can I switch back
>> to my previous solution? I can reduce the duplication of our
>> localized it66121 driver to a minimal, rewrite it until it meets
>> the community's requirement. I know our device looks weird and
>> our approach is not elegant. But at the very least, we could not
>> mess the community's design up by localize. Otherwise, I don't know
>> what is the better approach to solve such a problem.
>>
>> Can I switch back or any other ideas?
> I keep on repeating: create the i2c device from your root device
> driver, which parses BIOS data.


You didn't focus on solve the problem, You are focus on solving me.
How does the method that parsing BIOS data can be generic and applied
universally?

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Dmitry Baryshkov]

On Fri, 17 Nov 2023 at 19:36, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Hi,
>
>
> On 2023/11/17 17:03, Dmitry Baryshkov wrote:
> > On Fri, 17 Nov 2023 at 06:24, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> >> Hi,
> >>
> >> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
> >>>>>> Then you will need some way (fwnode?) to
> >>>>>> discover the bridge chain. And at the last point you will get into the
> >>>>>> device data and/or properties business.
> >>>>>>
> >>>>> No, leave that chance to a more better programmer and forgive me please,
> >>>>> too difficult, I'm afraid of not able to solve. Thanks a lot for the
> >>>>> trust!
> >>>   From my point of view: no.
> >>
> >> I respect the fact that the community prefer generic mechanisms.
> >> If our approach is not what the community want, can I switch back
> >> to my previous solution? I can reduce the duplication of our
> >> localized it66121 driver to a minimal, rewrite it until it meets
> >> the community's requirement. I know our device looks weird and
> >> our approach is not elegant. But at the very least, we could not
> >> mess the community's design up by localize. Otherwise, I don't know
> >> what is the better approach to solve such a problem.
> >>
> >> Can I switch back or any other ideas?
> > I keep on repeating: create the i2c device from your root device
> > driver, which parses BIOS data.
>
>
> You didn't focus on solve the problem, You are focus on solving me.
> How does the method that parsing BIOS data can be generic and applied
> universally?

Parsing BIOS data is unique to your platform (as well as your BIOS
tables). However using and extending (instead of replacing it just for
your platform) is a generic item.

-- 
With best wishes
Dmitry

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2185 bytes --]

On Fri, Nov 17, 2023 at 12:24:22PM +0800, Sui Jingfeng wrote:
> Hi,
> 
> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
> > > > > Then you will need some way (fwnode?) to
> > > > > discover the bridge chain. And at the last point you will get into the
> > > > > device data and/or properties business.
> > > > > 
> > > > No, leave that chance to a more better programmer and forgive me please,
> > > > too difficult, I'm afraid of not able to solve. Thanks a lot for the
> > > > trust!
> >  From my point of view: no.
>
> I respect the fact that the community prefer generic mechanisms.
> If our approach is not what the community want, can I switch back
> to my previous solution?

By your previous solution, you mean rolling your own bridge driver? If
so, then no, it's not acceptable either.

> I can reduce the duplication of our localized it66121 driver to a
> minimal, rewrite it until it meets the community's requirement. I know
> our device looks weird and our approach is not elegant.

I'm glad we agree then :)

> But at the very least, we could not mess the community's design up by
> localize. Otherwise, I don't know what is the better approach to solve
> such a problem.

I think there's a gap between what we want from you and what you want
from us.

What we really care about is maintenance. In other words, it's mostly
about two things:

  - Once you and/or your company have moved on to other things, how easy
    it will be for us to keep that driver in good shape, and how much it
    will hold back any future development.

  - If we want to do a big rework, how much your driver will stand in
    the way.

That's pretty much all that we care about, and we will very much prefer
not to merge a driver in the first place than to have to maintain it for
10y while it stands in our way and we don't have any real documentation
or help.

So by making it "not weird" or "elegant" or whatever we can call it, you
effectively remove any concern we might have about merging your driver,
and there's only an upside (more hardware support and company
involvement is good!). So you're making it easy for you too.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[kernel test robot]

Hi Sui,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.7-rc1 next-20231115]
[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/Sui-Jingfeng/drm-bridge-it66121-Use-dev-replace-ctx-dev-in-the-it66121_probe/20231114-231203
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20231114150130.497915-9-sui.jingfeng%40linux.dev
patch subject: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib
config: x86_64-randconfig-005-20231115 (https://download.01.org/0day-ci/archive/20231115/202311152059.KuAZPGVE-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231115/202311152059.KuAZPGVE-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/202311152059.KuAZPGVE-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/bridge/ite-it66121.c:1654:5: warning: no previous prototype for 'it66121_create_bridge' [-Wmissing-prototypes]
    1654 | int it66121_create_bridge(struct i2c_client *client, bool of_support,
         |     ^~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/bridge/ite-it66121.c:1752:6: warning: no previous prototype for 'it66121_destroy_bridge' [-Wmissing-prototypes]
    1752 | void it66121_destroy_bridge(struct drm_bridge *bridge)
         |      ^~~~~~~~~~~~~~~~~~~~~~


vim +/it66121_create_bridge +1654 drivers/gpu/drm/bridge/ite-it66121.c

  1653	
> 1654	int it66121_create_bridge(struct i2c_client *client, bool of_support,
  1655				  bool hpd_support, bool audio_support,
  1656				  struct drm_bridge **bridge)
  1657	{
  1658		struct device *dev = &client->dev;
  1659		int ret;
  1660		struct it66121_ctx *ctx;
  1661	
  1662		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
  1663		if (!ctx)
  1664			return -ENOMEM;
  1665	
  1666		ctx->dev = dev;
  1667		ctx->client = client;
  1668		mutex_init(&ctx->lock);
  1669	
  1670		if (of_support) {
  1671			ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
  1672			if (ret)
  1673				return ret;
  1674	
  1675			ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
  1676			if (ret)
  1677				return ret;
  1678		} else {
  1679			ctx->bus_width = 24;
  1680			ctx->next_bridge = NULL;
  1681		}
  1682	
  1683		it66121_hw_reset(ctx);
  1684	
  1685		ctx->regmap = devm_regmap_init_i2c(client, &it66121_regmap_config);
  1686		if (IS_ERR(ctx->regmap))
  1687			return PTR_ERR(ctx->regmap);
  1688	
  1689		ret = it66121_read_chip_id(ctx, false);
  1690		if (ret)
  1691			return ret;
  1692	
  1693		ctx->info = it66121_get_match_data(ctx->vender_id, ctx->device_id);
  1694		if (!ctx->info)
  1695			return -ENODEV;
  1696	
  1697		if (hpd_support) {
  1698			ret = devm_request_threaded_irq(dev, client->irq, NULL,
  1699							it66121_irq_threaded_handler,
  1700							IRQF_ONESHOT, dev_name(dev),
  1701							ctx);
  1702			if (ret < 0) {
  1703				dev_err(dev, "Failed to request irq: %d\n", ret);
  1704				return ret;
  1705			}
  1706		}
  1707	
  1708		it66121_bridge_init_base(&ctx->bridge, dev->of_node, true);
  1709	
  1710		if (audio_support)
  1711			it66121_audio_codec_init(ctx, dev);
  1712	
  1713		*bridge = &ctx->bridge;
  1714	
  1715		dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
  1716			 ctx->vender_id, ctx->device_id, ctx->revision);
  1717	
  1718		return 0;
  1719	}
  1720	EXPORT_SYMBOL_GPL(it66121_create_bridge);
  1721	
  1722	static int it66121_probe(struct i2c_client *client)
  1723	{
  1724		struct device *dev = &client->dev;
  1725		struct it66121_ctx *ctx;
  1726		struct drm_bridge *bridge;
  1727		int ret;
  1728	
  1729		if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
  1730			dev_err(dev, "I2C check functionality failed.\n");
  1731			return -ENXIO;
  1732		}
  1733	
  1734		ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it66121_supplies),
  1735						     it66121_supplies);
  1736		if (ret) {
  1737			dev_err(dev, "Failed to enable power supplies\n");
  1738			return ret;
  1739		}
  1740	
  1741		ret = it66121_create_bridge(client, true, true, true, &bridge);
  1742		if (ret)
  1743			return ret;
  1744	
  1745		ctx = bridge_to_it66121(bridge);
  1746	
  1747		i2c_set_clientdata(client, ctx);
  1748	
  1749		return 0;
  1750	}
  1751	
> 1752	void it66121_destroy_bridge(struct drm_bridge *bridge)
  1753	{
  1754		struct it66121_ctx *ctx = bridge_to_it66121(bridge);
  1755	
  1756		drm_bridge_remove(bridge);
  1757	
  1758		mutex_destroy(&ctx->lock);
  1759	}
  1760	EXPORT_SYMBOL_GPL(it66121_destroy_bridge);
  1761	

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

Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[kernel test robot]

Hi Sui,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.7-rc2 next-20231120]
[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/Sui-Jingfeng/drm-bridge-it66121-Use-dev-replace-ctx-dev-in-the-it66121_probe/20231114-231203
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20231114150130.497915-9-sui.jingfeng%40linux.dev
patch subject: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib
config: x86_64-buildonly-randconfig-004-20231120 (https://download.01.org/0day-ci/archive/20231120/202311201649.qjEbx5YL-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231120/202311201649.qjEbx5YL-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/202311201649.qjEbx5YL-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/bridge/ite-it66121.c:1654:5: warning: no previous prototype for function 'it66121_create_bridge' [-Wmissing-prototypes]
   int it66121_create_bridge(struct i2c_client *client, bool of_support,
       ^
   drivers/gpu/drm/bridge/ite-it66121.c:1654:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int it66121_create_bridge(struct i2c_client *client, bool of_support,
   ^
   static 
>> drivers/gpu/drm/bridge/ite-it66121.c:1752:6: warning: no previous prototype for function 'it66121_destroy_bridge' [-Wmissing-prototypes]
   void it66121_destroy_bridge(struct drm_bridge *bridge)
        ^
   drivers/gpu/drm/bridge/ite-it66121.c:1752:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void it66121_destroy_bridge(struct drm_bridge *bridge)
   ^
   static 
   2 warnings generated.


vim +/it66121_create_bridge +1654 drivers/gpu/drm/bridge/ite-it66121.c

  1653	
> 1654	int it66121_create_bridge(struct i2c_client *client, bool of_support,
  1655				  bool hpd_support, bool audio_support,
  1656				  struct drm_bridge **bridge)
  1657	{
  1658		struct device *dev = &client->dev;
  1659		int ret;
  1660		struct it66121_ctx *ctx;
  1661	
  1662		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
  1663		if (!ctx)
  1664			return -ENOMEM;
  1665	
  1666		ctx->dev = dev;
  1667		ctx->client = client;
  1668		mutex_init(&ctx->lock);
  1669	
  1670		if (of_support) {
  1671			ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
  1672			if (ret)
  1673				return ret;
  1674	
  1675			ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
  1676			if (ret)
  1677				return ret;
  1678		} else {
  1679			ctx->bus_width = 24;
  1680			ctx->next_bridge = NULL;
  1681		}
  1682	
  1683		it66121_hw_reset(ctx);
  1684	
  1685		ctx->regmap = devm_regmap_init_i2c(client, &it66121_regmap_config);
  1686		if (IS_ERR(ctx->regmap))
  1687			return PTR_ERR(ctx->regmap);
  1688	
  1689		ret = it66121_read_chip_id(ctx, false);
  1690		if (ret)
  1691			return ret;
  1692	
  1693		ctx->info = it66121_get_match_data(ctx->vender_id, ctx->device_id);
  1694		if (!ctx->info)
  1695			return -ENODEV;
  1696	
  1697		if (hpd_support) {
  1698			ret = devm_request_threaded_irq(dev, client->irq, NULL,
  1699							it66121_irq_threaded_handler,
  1700							IRQF_ONESHOT, dev_name(dev),
  1701							ctx);
  1702			if (ret < 0) {
  1703				dev_err(dev, "Failed to request irq: %d\n", ret);
  1704				return ret;
  1705			}
  1706		}
  1707	
  1708		it66121_bridge_init_base(&ctx->bridge, dev->of_node, true);
  1709	
  1710		if (audio_support)
  1711			it66121_audio_codec_init(ctx, dev);
  1712	
  1713		*bridge = &ctx->bridge;
  1714	
  1715		dev_info(dev, "IT66121 probed, chip id: 0x%x:0x%x, revision: %u\n",
  1716			 ctx->vender_id, ctx->device_id, ctx->revision);
  1717	
  1718		return 0;
  1719	}
  1720	EXPORT_SYMBOL_GPL(it66121_create_bridge);
  1721	
  1722	static int it66121_probe(struct i2c_client *client)
  1723	{
  1724		struct device *dev = &client->dev;
  1725		struct it66121_ctx *ctx;
  1726		struct drm_bridge *bridge;
  1727		int ret;
  1728	
  1729		if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
  1730			dev_err(dev, "I2C check functionality failed.\n");
  1731			return -ENXIO;
  1732		}
  1733	
  1734		ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it66121_supplies),
  1735						     it66121_supplies);
  1736		if (ret) {
  1737			dev_err(dev, "Failed to enable power supplies\n");
  1738			return ret;
  1739		}
  1740	
  1741		ret = it66121_create_bridge(client, true, true, true, &bridge);
  1742		if (ret)
  1743			return ret;
  1744	
  1745		ctx = bridge_to_it66121(bridge);
  1746	
  1747		i2c_set_clientdata(client, ctx);
  1748	
  1749		return 0;
  1750	}
  1751	
> 1752	void it66121_destroy_bridge(struct drm_bridge *bridge)
  1753	{
  1754		struct it66121_ctx *ctx = bridge_to_it66121(bridge);
  1755	
  1756		drm_bridge_remove(bridge);
  1757	
  1758		mutex_destroy(&ctx->lock);
  1759	}
  1760	EXPORT_SYMBOL_GPL(it66121_destroy_bridge);
  1761	

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