Currently, the various drm bridge drivers relay on OF infrastructures to
works very well. Yet there are platforms and/or don not has OF support.
Such as virtual display drivers, USB display apapters and ACPI based
systems etc. Add fwnode based helpers to fill the niche, this may allows
part of the drm display bridge drivers to work across systems. As the
fwnode based API has wider coverage than DT, it can be used on all systems
in theory. Assumed that the system has valid fwnode graphs established
before drm bridge driver is probed, the fwnode graphs are compatible with
the OF graph.
Tested on TI BeaglePlay board and other platforms.
v1 at https://patchwork.freedesktop.org/series/129040/
v2:
* Modify it66121 to switch togather
* Drop the 'side-by-side' implement
* Add drm_bridge_find_next_bridge_by_fwnode() helper
* Add drm_bridge_set_node() helper
Sui Jingfeng (4):
drm/bridge: Add fwnode based helpers to get the next bridge
drm/bridge: simple-bridge: Use fwnode API to acquire device properties
drm-bridge: display-connector: Use fwnode API to acquire device
properties
drm-bridge: it66121: Use fwnode API to acquire device properties
drivers/gpu/drm/bridge/display-connector.c | 24 ++++----
drivers/gpu/drm/bridge/ite-it66121.c | 63 +++++++++++---------
drivers/gpu/drm/bridge/simple-bridge.c | 22 ++++---
drivers/gpu/drm/drm_bridge.c | 68 ++++++++++++++++++++++
include/drm/drm_bridge.h | 16 +++++
5 files changed, 142 insertions(+), 51 deletions(-)
--
2.34.1
Currently, the various drm bridge drivers relay on OF infrastructures to
works very well. Yet there are platforms and/or don not has OF support.
Such as virtual display drivers, USB display apapters and ACPI based
systems etc. Add fwnode based helpers to fill the niche, this may allows
part of the drm display bridge drivers to work across systems. As the
fwnode based API has wider coverage than DT, it can be used on all systems
in theory. Assumed that the system has valid fwnode graphs established
before drm bridge driver is probed, the fwnode graphs are compatible with
the OF graph.
Signed-off-by: Sui Jingfeng <[email protected]>
---
drivers/gpu/drm/drm_bridge.c | 68 ++++++++++++++++++++++++++++++++++++
include/drm/drm_bridge.h | 16 +++++++++
2 files changed, 84 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 521a71c61b16..1b2d3b89a61d 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1348,6 +1348,74 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
EXPORT_SYMBOL(of_drm_find_bridge);
#endif
+/**
+ * drm_bridge_find_by_fwnode - Find the bridge corresponding to the associated fwnode
+ *
+ * @fwnode: fwnode for which to find the matching drm_bridge
+ *
+ * This function looks up a drm_bridge based on its associated fwnode.
+ *
+ * RETURNS:
+ * A reference to the drm_bridge if found, otherwise return NULL.
+ */
+struct drm_bridge *drm_bridge_find_by_fwnode(struct fwnode_handle *fwnode)
+{
+ struct drm_bridge *ret = NULL;
+ struct drm_bridge *bridge;
+
+ if (!fwnode)
+ return NULL;
+
+ mutex_lock(&bridge_lock);
+
+ list_for_each_entry(bridge, &bridge_list, list) {
+ if (bridge->fwnode == fwnode) {
+ ret = bridge;
+ break;
+ }
+ }
+
+ mutex_unlock(&bridge_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL(drm_bridge_find_by_fwnode);
+
+/**
+ * drm_bridge_find_next_bridge_by_fwnode - get the next bridge by fwnode
+ * @fwnode: fwnode pointer to the current bridge.
+ * @port: identifier of the port node of the next bridge is connected.
+ *
+ * This function find the next bridge at the current bridge node, assumed
+ * that there has valid fwnode graph established.
+ *
+ * RETURNS:
+ * A reference to the drm_bridge if found, %NULL if not found.
+ * Otherwise return a negative error code.
+ */
+struct drm_bridge *
+drm_bridge_find_next_bridge_by_fwnode(struct fwnode_handle *fwnode, u32 port)
+{
+ struct drm_bridge *bridge;
+ struct fwnode_handle *ep;
+ struct fwnode_handle *remote;
+
+ ep = fwnode_graph_get_endpoint_by_id(fwnode, port, 0, 0);
+ if (!ep)
+ return ERR_PTR(-EINVAL);
+
+ remote = fwnode_graph_get_remote_port_parent(ep);
+ fwnode_handle_put(ep);
+ if (!remote)
+ return ERR_PTR(-ENODEV);
+
+ bridge = drm_bridge_find_by_fwnode(remote);
+ fwnode_handle_put(remote);
+
+ return bridge;
+}
+EXPORT_SYMBOL(drm_bridge_find_next_bridge_by_fwnode);
+
MODULE_AUTHOR("Ajay Kumar <[email protected]>");
MODULE_DESCRIPTION("DRM bridge infrastructure");
MODULE_LICENSE("GPL and additional rights");
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3606e1a7f965..d4c95afdd662 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -26,6 +26,7 @@
#include <linux/ctype.h>
#include <linux/list.h>
#include <linux/mutex.h>
+#include <linux/of.h>
#include <drm/drm_atomic.h>
#include <drm/drm_encoder.h>
@@ -721,6 +722,8 @@ struct drm_bridge {
struct list_head chain_node;
/** @of_node: device node pointer to the bridge */
struct device_node *of_node;
+ /** @fwnode: fwnode pointer to the bridge */
+ struct fwnode_handle *fwnode;
/** @list: to keep track of all added bridges */
struct list_head list;
/**
@@ -788,6 +791,13 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
struct drm_bridge *previous,
enum drm_bridge_attach_flags flags);
+static inline void
+drm_bridge_set_node(struct drm_bridge *bridge, struct fwnode_handle *fwnode)
+{
+ bridge->fwnode = fwnode;
+ bridge->of_node = to_of_node(fwnode);
+}
+
#ifdef CONFIG_OF
struct drm_bridge *of_drm_find_bridge(struct device_node *np);
#else
@@ -797,6 +807,12 @@ static inline struct drm_bridge *of_drm_find_bridge(struct device_node *np)
}
#endif
+struct drm_bridge *
+drm_bridge_find_by_fwnode(struct fwnode_handle *fwnode);
+
+struct drm_bridge *
+drm_bridge_find_next_bridge_by_fwnode(struct fwnode_handle *fwnode, u32 port);
+
/**
* drm_bridge_get_next_bridge() - Get the next bridge in the chain
* @bridge: bridge object
--
2.34.1
Make this driver less DT-dependent by calling the freshly created helpers,
should be no functional changes for DT based systems. But open the door for
otherwise use cases. Even though there is no user emerged yet, this still
do no harms.
Signed-off-by: Sui Jingfeng <[email protected]>
---
drivers/gpu/drm/bridge/simple-bridge.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
index 5813a2c4fc5e..bf00b9b5e00f 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -9,7 +9,6 @@
#include <linux/gpio/consumer.h>
#include <linux/module.h>
#include <linux/of.h>
-#include <linux/of_graph.h>
#include <linux/platform_device.h>
#include <linux/regulator/consumer.h>
@@ -169,33 +168,32 @@ static const struct drm_bridge_funcs simple_bridge_bridge_funcs = {
static int simple_bridge_probe(struct platform_device *pdev)
{
+ struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
struct simple_bridge *sbridge;
- struct device_node *remote;
+ int ret;
sbridge = devm_kzalloc(&pdev->dev, sizeof(*sbridge), GFP_KERNEL);
if (!sbridge)
return -ENOMEM;
platform_set_drvdata(pdev, sbridge);
- sbridge->info = of_device_get_match_data(&pdev->dev);
+ sbridge->info = device_get_match_data(&pdev->dev);
/* Get the next bridge in the pipeline. */
- remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
- if (!remote)
- return -EINVAL;
-
- sbridge->next_bridge = of_drm_find_bridge(remote);
- of_node_put(remote);
-
+ sbridge->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
if (!sbridge->next_bridge) {
dev_dbg(&pdev->dev, "Next bridge not found, deferring probe\n");
return -EPROBE_DEFER;
+ } else if (IS_ERR(sbridge->next_bridge)) {
+ ret = PTR_ERR(sbridge->next_bridge);
+ dev_err(&pdev->dev, "Error on find the next bridge: %d\n", ret);
+ return ret;
}
/* Get the regulator and GPIO resources. */
sbridge->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
if (IS_ERR(sbridge->vdd)) {
- int ret = PTR_ERR(sbridge->vdd);
+ ret = PTR_ERR(sbridge->vdd);
if (ret == -EPROBE_DEFER)
return -EPROBE_DEFER;
sbridge->vdd = NULL;
@@ -210,9 +208,9 @@ static int simple_bridge_probe(struct platform_device *pdev)
/* Register the bridge. */
sbridge->bridge.funcs = &simple_bridge_bridge_funcs;
- sbridge->bridge.of_node = pdev->dev.of_node;
sbridge->bridge.timings = sbridge->info->timings;
+ drm_bridge_set_node(&sbridge->bridge, fwnode);
drm_bridge_add(&sbridge->bridge);
return 0;
--
2.34.1
Make this driver less DT-dependent by using the fwnode helper function,
should be no functional changes for DT based systems. Do the necessary
works before it can be truely subsystem independent, even though there
is no user yet, this still do no harms.
Signed-off-by: Sui Jingfeng <[email protected]>
---
drivers/gpu/drm/bridge/display-connector.c | 24 +++++++++++-----------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
index ab8e00baf3f1..d80cb7bc59e6 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -204,6 +204,7 @@ static int display_connector_get_supply(struct platform_device *pdev,
static int display_connector_probe(struct platform_device *pdev)
{
+ struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
struct display_connector *conn;
unsigned int type;
const char *label = NULL;
@@ -215,15 +216,15 @@ static int display_connector_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, conn);
- type = (uintptr_t)of_device_get_match_data(&pdev->dev);
+ type = (uintptr_t)device_get_match_data(&pdev->dev);
/* Get the exact connector type. */
switch (type) {
case DRM_MODE_CONNECTOR_DVII: {
bool analog, digital;
- analog = of_property_read_bool(pdev->dev.of_node, "analog");
- digital = of_property_read_bool(pdev->dev.of_node, "digital");
+ analog = fwnode_property_present(fwnode, "analog");
+ digital = fwnode_property_present(fwnode, "digital");
if (analog && !digital) {
conn->bridge.type = DRM_MODE_CONNECTOR_DVIA;
} else if (!analog && digital) {
@@ -240,8 +241,7 @@ static int display_connector_probe(struct platform_device *pdev)
case DRM_MODE_CONNECTOR_HDMIA: {
const char *hdmi_type;
- ret = of_property_read_string(pdev->dev.of_node, "type",
- &hdmi_type);
+ ret = fwnode_property_read_string(fwnode, "type", &hdmi_type);
if (ret < 0) {
dev_err(&pdev->dev, "HDMI connector with no type\n");
return -EINVAL;
@@ -271,7 +271,7 @@ static int display_connector_probe(struct platform_device *pdev)
conn->bridge.interlace_allowed = true;
/* Get the optional connector label. */
- of_property_read_string(pdev->dev.of_node, "label", &label);
+ fwnode_property_read_string(fwnode, "label", &label);
/*
* Get the HPD GPIO for DVI, HDMI and DP connectors. If the GPIO can provide
@@ -309,12 +309,12 @@ static int display_connector_probe(struct platform_device *pdev)
if (type == DRM_MODE_CONNECTOR_DVII ||
type == DRM_MODE_CONNECTOR_HDMIA ||
type == DRM_MODE_CONNECTOR_VGA) {
- struct device_node *phandle;
+ struct fwnode_handle *phandle;
- phandle = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0);
- if (phandle) {
- conn->bridge.ddc = of_get_i2c_adapter_by_node(phandle);
- of_node_put(phandle);
+ phandle = fwnode_find_reference(fwnode, "ddc-i2c-bus", 0);
+ if (!IS_ERR_OR_NULL(phandle)) {
+ conn->bridge.ddc = i2c_get_adapter_by_fwnode(phandle);
+ fwnode_handle_put(phandle);
if (!conn->bridge.ddc)
return -EPROBE_DEFER;
} else {
@@ -358,7 +358,7 @@ static int display_connector_probe(struct platform_device *pdev)
}
conn->bridge.funcs = &display_connector_bridge_funcs;
- conn->bridge.of_node = pdev->dev.of_node;
+ drm_bridge_set_node(&conn->bridge, fwnode);
if (conn->bridge.ddc)
conn->bridge.ops |= DRM_BRIDGE_OP_EDID
--
2.34.1
Make this driver less DT-dependent by calling the freshly created helpers,
should be no functional changes for DT based systems. But open the door for
otherwise use cases. Even though there is no user emerged yet, this still
do no harms. In fact, we reduce some boilerplate across drm bridge drivers.
Signed-off-by: Sui Jingfeng <[email protected]>
---
drivers/gpu/drm/bridge/ite-it66121.c | 63 ++++++++++++++++------------
1 file changed, 36 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 1c3433b5e366..a2cf2be86065 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -15,7 +15,6 @@
#include <linux/bitfield.h>
#include <linux/property.h>
#include <linux/regmap.h>
-#include <linux/of_graph.h>
#include <linux/gpio/consumer.h>
#include <linux/pinctrl/consumer.h>
#include <linux/regulator/consumer.h>
@@ -1480,7 +1479,7 @@ static int it66121_audio_codec_init(struct it66121_ctx *ctx, struct device *dev)
dev_dbg(dev, "%s\n", __func__);
- if (!of_property_read_bool(dev->of_node, "#sound-dai-cells")) {
+ if (!fwnode_property_present(dev_fwnode(dev), "#sound-dai-cells")) {
dev_info(dev, "No \"#sound-dai-cells\", no audio\n");
return 0;
}
@@ -1503,13 +1502,37 @@ static const char * const it66121_supplies[] = {
"vcn33", "vcn18", "vrf12"
};
+static int it66121_read_bus_width(struct fwnode_handle *fwnode, u32 port,
+ u32 *bus_width)
+{
+ struct fwnode_handle *endpoint;
+ u32 val;
+ int ret;
+
+ endpoint = fwnode_graph_get_endpoint_by_id(fwnode, port, 0, 0);
+ if (!endpoint)
+ return -EINVAL;
+
+ ret = fwnode_property_read_u32(endpoint, "bus-width", &val);
+ fwnode_handle_put(endpoint);
+ if (ret)
+ return ret;
+
+ if (val != 12 && val != 24)
+ return -EINVAL;
+
+ *bus_width = val;
+
+ return 0;
+}
+
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;
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
dev_err(dev, "I2C check functionality failed.\n");
@@ -1520,37 +1543,23 @@ 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;
-
- ep = of_graph_get_remote_node(dev->of_node, 1, -1);
- if (!ep) {
- dev_err(ctx->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");
- return -ENODEV;
- }
+ /* Endpoint of port@0 contains the bus-width property */
+ ret = it66121_read_bus_width(fwnode, 0, &ctx->bus_width);
+ if (ret)
+ return ret;
- ctx->next_bridge = of_drm_find_bridge(ep);
- of_node_put(ep);
+ ctx->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
if (!ctx->next_bridge) {
dev_dbg(ctx->dev, "Next bridge not found, deferring probe\n");
return -EPROBE_DEFER;
+ } else if (IS_ERR(ctx->next_bridge)) {
+ ret = PTR_ERR(ctx->next_bridge);
+ dev_err(dev, "Error in founding the next bridge: %d\n", ret);
+ return ret;
}
i2c_set_clientdata(client, ctx);
@@ -1584,9 +1593,9 @@ static int it66121_probe(struct i2c_client *client)
}
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;
+ drm_bridge_set_node(&ctx->bridge, fwnode);
ret = devm_request_threaded_irq(dev, client->irq, NULL, it66121_irq_threaded_handler,
IRQF_ONESHOT, dev_name(dev), ctx);
--
2.34.1
On Thu, 7 Mar 2024 at 19:23, Sui Jingfeng <[email protected]> wrote:
>
> Currently, the various drm bridge drivers relay on OF infrastructures to
> works very well. Yet there are platforms and/or don not has OF support.
> Such as virtual display drivers, USB display apapters and ACPI based
> systems etc. Add fwnode based helpers to fill the niche, this may allows
> part of the drm display bridge drivers to work across systems. As the
> fwnode based API has wider coverage than DT, it can be used on all systems
> in theory. Assumed that the system has valid fwnode graphs established
> before drm bridge driver is probed, the fwnode graphs are compatible with
> the OF graph.
>
> Signed-off-by: Sui Jingfeng <[email protected]>
> ---
> drivers/gpu/drm/drm_bridge.c | 68 ++++++++++++++++++++++++++++++++++++
> include/drm/drm_bridge.h | 16 +++++++++
> 2 files changed, 84 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 521a71c61b16..1b2d3b89a61d 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1348,6 +1348,74 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
> EXPORT_SYMBOL(of_drm_find_bridge);
> #endif
>
> +/**
> + * drm_bridge_find_by_fwnode - Find the bridge corresponding to the associated fwnode
> + *
> + * @fwnode: fwnode for which to find the matching drm_bridge
> + *
> + * This function looks up a drm_bridge based on its associated fwnode.
> + *
> + * RETURNS:
> + * A reference to the drm_bridge if found, otherwise return NULL.
> + */
Please take a look at Documentation/doc-guide/kernel-doc.rst.
> +struct drm_bridge *drm_bridge_find_by_fwnode(struct fwnode_handle *fwnode)
> +{
> + struct drm_bridge *ret = NULL;
> + struct drm_bridge *bridge;
> +
> + if (!fwnode)
> + return NULL;
> +
> + mutex_lock(&bridge_lock);
> +
> + list_for_each_entry(bridge, &bridge_list, list) {
> + if (bridge->fwnode == fwnode) {
> + ret = bridge;
> + break;
> + }
> + }
> +
> + mutex_unlock(&bridge_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_bridge_find_by_fwnode);
EXPORT_SYMBOL_GPL
> +
> +/**
> + * drm_bridge_find_next_bridge_by_fwnode - get the next bridge by fwnode
> + * @fwnode: fwnode pointer to the current bridge.
> + * @port: identifier of the port node of the next bridge is connected.
> + *
> + * This function find the next bridge at the current bridge node, assumed
> + * that there has valid fwnode graph established.
> + *
> + * RETURNS:
> + * A reference to the drm_bridge if found, %NULL if not found.
> + * Otherwise return a negative error code.
> + */
> +struct drm_bridge *
> +drm_bridge_find_next_bridge_by_fwnode(struct fwnode_handle *fwnode, u32 port)
> +{
> + struct drm_bridge *bridge;
> + struct fwnode_handle *ep;
> + struct fwnode_handle *remote;
> +
> + ep = fwnode_graph_get_endpoint_by_id(fwnode, port, 0, 0);
> + if (!ep)
> + return ERR_PTR(-EINVAL);
> +
> + remote = fwnode_graph_get_remote_port_parent(ep);
> + fwnode_handle_put(ep);
> + if (!remote)
> + return ERR_PTR(-ENODEV);
> +
> + bridge = drm_bridge_find_by_fwnode(remote);
> + fwnode_handle_put(remote);
> +
> + return bridge;
> +}
> +EXPORT_SYMBOL(drm_bridge_find_next_bridge_by_fwnode);
EXPORT_SYMBOL_GPL
> +
> MODULE_AUTHOR("Ajay Kumar <[email protected]>");
> MODULE_DESCRIPTION("DRM bridge infrastructure");
> MODULE_LICENSE("GPL and additional rights");
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3606e1a7f965..d4c95afdd662 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -26,6 +26,7 @@
> #include <linux/ctype.h>
> #include <linux/list.h>
> #include <linux/mutex.h>
> +#include <linux/of.h>
>
> #include <drm/drm_atomic.h>
> #include <drm/drm_encoder.h>
> @@ -721,6 +722,8 @@ struct drm_bridge {
> struct list_head chain_node;
> /** @of_node: device node pointer to the bridge */
> struct device_node *of_node;
In my opinion, if you are adding fwnode, we can drop of_node
completely. There is no need to keep both of them.
> + /** @fwnode: fwnode pointer to the bridge */
> + struct fwnode_handle *fwnode;
> /** @list: to keep track of all added bridges */
> struct list_head list;
> /**
> @@ -788,6 +791,13 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> struct drm_bridge *previous,
> enum drm_bridge_attach_flags flags);
>
> +static inline void
> +drm_bridge_set_node(struct drm_bridge *bridge, struct fwnode_handle *fwnode)
> +{
> + bridge->fwnode = fwnode;
> + bridge->of_node = to_of_node(fwnode);
> +}
> +
> #ifdef CONFIG_OF
> struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> #else
> @@ -797,6 +807,12 @@ static inline struct drm_bridge *of_drm_find_bridge(struct device_node *np)
> }
> #endif
>
> +struct drm_bridge *
> +drm_bridge_find_by_fwnode(struct fwnode_handle *fwnode);
> +
> +struct drm_bridge *
> +drm_bridge_find_next_bridge_by_fwnode(struct fwnode_handle *fwnode, u32 port);
> +
> /**
> * drm_bridge_get_next_bridge() - Get the next bridge in the chain
> * @bridge: bridge object
> --
> 2.34.1
>
--
With best wishes
Dmitry
Hi,
On 2024/3/8 02:43, Dmitry Baryshkov wrote:
>> +
>> MODULE_AUTHOR("Ajay Kumar<[email protected]>");
>> MODULE_DESCRIPTION("DRM bridge infrastructure");
>> MODULE_LICENSE("GPL and additional rights");
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 3606e1a7f965..d4c95afdd662 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -26,6 +26,7 @@
>> #include <linux/ctype.h>
>> #include <linux/list.h>
>> #include <linux/mutex.h>
>> +#include <linux/of.h>
>>
>> #include <drm/drm_atomic.h>
>> #include <drm/drm_encoder.h>
>> @@ -721,6 +722,8 @@ struct drm_bridge {
>> struct list_head chain_node;
>> /** @of_node: device node pointer to the bridge */
>> struct device_node *of_node;
> In my opinion, if you are adding fwnode, we can drop of_node
> completely. There is no need to keep both of them.
But the 'struct device' have both of them contained, we should *follow the core*, right?
They are two major firmware subsystems (DT and ACPT), both are great and large, right?
Personally, I think the drm_bridge should embeds 'struct device', after all, drm bridge
are mainly stand for display bridges device. And also to reflect what you said: "to
reflect the hardware perfectly" and remove some boilerplate.
I think I'm not good enough to do such a big refactor, sorry. I believe that Maxime
and Laurent are the advanced programmers who is good enough to do such things, maybe
you can ask them for help?
Beside this, other reviews are acceptable and will be fixed at the next version,
thanks a lot for review.
Best Regards,
Sui
On 2024/3/8 03:20, Sui Jingfeng wrote:
> I think the drm_bridge should embeds 'struct device'
'struct device' -> 'struct device *'
On Thu, 7 Mar 2024 at 19:24, Sui Jingfeng <[email protected]> wrote:
>
> Make this driver less DT-dependent by calling the freshly created helpers,
> should be no functional changes for DT based systems. But open the door for
> otherwise use cases. Even though there is no user emerged yet, this still
> do no harms. In fact, we reduce some boilerplate across drm bridge drivers.
>
> Signed-off-by: Sui Jingfeng <[email protected]>
> ---
> drivers/gpu/drm/bridge/ite-it66121.c | 63 ++++++++++++++++------------
> 1 file changed, 36 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 1c3433b5e366..a2cf2be86065 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -15,7 +15,6 @@
> #include <linux/bitfield.h>
> #include <linux/property.h>
> #include <linux/regmap.h>
> -#include <linux/of_graph.h>
> #include <linux/gpio/consumer.h>
> #include <linux/pinctrl/consumer.h>
> #include <linux/regulator/consumer.h>
> @@ -1480,7 +1479,7 @@ static int it66121_audio_codec_init(struct it66121_ctx *ctx, struct device *dev)
>
> dev_dbg(dev, "%s\n", __func__);
>
> - if (!of_property_read_bool(dev->of_node, "#sound-dai-cells")) {
> + if (!fwnode_property_present(dev_fwnode(dev), "#sound-dai-cells")) {
> dev_info(dev, "No \"#sound-dai-cells\", no audio\n");
> return 0;
> }
> @@ -1503,13 +1502,37 @@ static const char * const it66121_supplies[] = {
> "vcn33", "vcn18", "vrf12"
> };
>
> +static int it66121_read_bus_width(struct fwnode_handle *fwnode, u32 port,
> + u32 *bus_width)
> +{
> + struct fwnode_handle *endpoint;
> + u32 val;
> + int ret;
> +
> + endpoint = fwnode_graph_get_endpoint_by_id(fwnode, port, 0, 0);
> + if (!endpoint)
> + return -EINVAL;
> +
> + ret = fwnode_property_read_u32(endpoint, "bus-width", &val);
> + fwnode_handle_put(endpoint);
> + if (ret)
> + return ret;
> +
> + if (val != 12 && val != 24)
> + return -EINVAL;
> +
> + *bus_width = val;
> +
> + return 0;
> +}
> +
> 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;
> + struct fwnode_handle *fwnode = dev_fwnode(dev);
>
> if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> dev_err(dev, "I2C check functionality failed.\n");
> @@ -1520,37 +1543,23 @@ 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;
> -
> - ep = of_graph_get_remote_node(dev->of_node, 1, -1);
> - if (!ep) {
> - dev_err(ctx->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");
> - return -ENODEV;
> - }
> + /* Endpoint of port@0 contains the bus-width property */
> + ret = it66121_read_bus_width(fwnode, 0, &ctx->bus_width);
There is no need to pass port as an argument to that function.
> + if (ret)
> + return ret;
>
> - ctx->next_bridge = of_drm_find_bridge(ep);
> - of_node_put(ep);
> + ctx->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
> if (!ctx->next_bridge) {
> dev_dbg(ctx->dev, "Next bridge not found, deferring probe\n");
> return -EPROBE_DEFER;
> + } else if (IS_ERR(ctx->next_bridge)) {
> + ret = PTR_ERR(ctx->next_bridge);
> + dev_err(dev, "Error in founding the next bridge: %d\n", ret);
> + return ret;
Nit: I'd usually expect this part to be in a different order: first
check for error, then check for absence. But that's a minor thing.
> }
>
> i2c_set_clientdata(client, ctx);
> @@ -1584,9 +1593,9 @@ static int it66121_probe(struct i2c_client *client)
> }
>
> 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;
> + drm_bridge_set_node(&ctx->bridge, fwnode);
>
> ret = devm_request_threaded_irq(dev, client->irq, NULL, it66121_irq_threaded_handler,
> IRQF_ONESHOT, dev_name(dev), ctx);
> --
> 2.34.1
>
--
With best wishes
Dmitry
On Thu, 7 Mar 2024 at 21:20, Sui Jingfeng <[email protected]> wrote:
>
> Hi,
>
>
> On 2024/3/8 02:43, Dmitry Baryshkov wrote:
> >> +
> >> MODULE_AUTHOR("Ajay Kumar<[email protected]>");
> >> MODULE_DESCRIPTION("DRM bridge infrastructure");
> >> MODULE_LICENSE("GPL and additional rights");
> >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> >> index 3606e1a7f965..d4c95afdd662 100644
> >> --- a/include/drm/drm_bridge.h
> >> +++ b/include/drm/drm_bridge.h
> >> @@ -26,6 +26,7 @@
> >> #include <linux/ctype.h>
> >> #include <linux/list.h>
> >> #include <linux/mutex.h>
> >> +#include <linux/of.h>
> >>
> >> #include <drm/drm_atomic.h>
> >> #include <drm/drm_encoder.h>
> >> @@ -721,6 +722,8 @@ struct drm_bridge {
> >> struct list_head chain_node;
> >> /** @of_node: device node pointer to the bridge */
> >> struct device_node *of_node;
> > In my opinion, if you are adding fwnode, we can drop of_node
> > completely. There is no need to keep both of them.
>
>
> But the 'struct device' have both of them contained, we should *follow the core*, right?
> They are two major firmware subsystems (DT and ACPT), both are great and large, right?
> Personally, I think the drm_bridge should embeds 'struct device', after all, drm bridge
> are mainly stand for display bridges device. And also to reflect what you said: "to
> reflect the hardware perfectly" and remove some boilerplate.
struct device contains both because it is at the root of the hierarchy
and it should support both API. drm_bridge is a consumer, so it's fine
to have just one.
>
> I think I'm not good enough to do such a big refactor, sorry. I believe that Maxime
> and Laurent are the advanced programmers who is good enough to do such things, maybe
> you can ask them for help?
Well, you picked up the task ;-)
But really, there is nothing so hard about it:
- Change of_node to fw_node, apply an automatic patch changing this in
bridge drivers.
- Make drm_of_bridge functions convert passed of_node and comp
After this we can start cleaning up bridge drivers to use fw_node API
natively as you did in your patches 2-4.
>
> Beside this, other reviews are acceptable and will be fixed at the next version,
> thanks a lot for review.
>
>
> Best Regards,
> Sui
>
--
With best wishes
Dmitry
Hi,
On 2024/3/8 03:31, Dmitry Baryshkov wrote:
> On Thu, 7 Mar 2024 at 19:24, Sui Jingfeng <[email protected]> wrote:
>> Make this driver less DT-dependent by calling the freshly created helpers,
>> should be no functional changes for DT based systems. But open the door for
>> otherwise use cases. Even though there is no user emerged yet, this still
>> do no harms. In fact, we reduce some boilerplate across drm bridge drivers.
>>
>> Signed-off-by: Sui Jingfeng <[email protected]>
>> ---
>> drivers/gpu/drm/bridge/ite-it66121.c | 63 ++++++++++++++++------------
>> 1 file changed, 36 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
>> index 1c3433b5e366..a2cf2be86065 100644
>> --- a/drivers/gpu/drm/bridge/ite-it66121.c
>> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
>> @@ -15,7 +15,6 @@
>> #include <linux/bitfield.h>
>> #include <linux/property.h>
>> #include <linux/regmap.h>
>> -#include <linux/of_graph.h>
>> #include <linux/gpio/consumer.h>
>> #include <linux/pinctrl/consumer.h>
>> #include <linux/regulator/consumer.h>
>> @@ -1480,7 +1479,7 @@ static int it66121_audio_codec_init(struct it66121_ctx *ctx, struct device *dev)
>>
>> dev_dbg(dev, "%s\n", __func__);
>>
>> - if (!of_property_read_bool(dev->of_node, "#sound-dai-cells")) {
>> + if (!fwnode_property_present(dev_fwnode(dev), "#sound-dai-cells")) {
>> dev_info(dev, "No \"#sound-dai-cells\", no audio\n");
>> return 0;
>> }
>> @@ -1503,13 +1502,37 @@ static const char * const it66121_supplies[] = {
>> "vcn33", "vcn18", "vrf12"
>> };
>>
>> +static int it66121_read_bus_width(struct fwnode_handle *fwnode, u32 port,
>> + u32 *bus_width)
>> +{
>> + struct fwnode_handle *endpoint;
>> + u32 val;
>> + int ret;
>> +
>> + endpoint = fwnode_graph_get_endpoint_by_id(fwnode, port, 0, 0);
>> + if (!endpoint)
>> + return -EINVAL;
>> +
>> + ret = fwnode_property_read_u32(endpoint, "bus-width", &val);
>> + fwnode_handle_put(endpoint);
>> + if (ret)
>> + return ret;
>> +
>> + if (val != 12 && val != 24)
>> + return -EINVAL;
>> +
>> + *bus_width = val;
>> +
>> + return 0;
>> +}
>> +
>> 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;
>> + struct fwnode_handle *fwnode = dev_fwnode(dev);
>>
>> if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> dev_err(dev, "I2C check functionality failed.\n");
>> @@ -1520,37 +1543,23 @@ 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;
>> -
>> - ep = of_graph_get_remote_node(dev->of_node, 1, -1);
>> - if (!ep) {
>> - dev_err(ctx->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");
>> - return -ENODEV;
>> - }
>> + /* Endpoint of port@0 contains the bus-width property */
>> + ret = it66121_read_bus_width(fwnode, 0, &ctx->bus_width);
> There is no need to pass port as an argument to that function.
>
>
Yeah, extremely correct. Because the bus width property should always
located at the endpoint of the input port(port@0 for it66121).
>> + if (ret)
>> + return ret;
>>
>> - ctx->next_bridge = of_drm_find_bridge(ep);
>> - of_node_put(ep);
>> + ctx->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
>> if (!ctx->next_bridge) {
>> dev_dbg(ctx->dev, "Next bridge not found, deferring probe\n");
>> return -EPROBE_DEFER;
>> + } else if (IS_ERR(ctx->next_bridge)) {
>> + ret = PTR_ERR(ctx->next_bridge);
>> + dev_err(dev, "Error in founding the next bridge: %d\n", ret);
>> + return ret;
> Nit: I'd usually expect this part to be in a different order: first
> check for error, then check for absence. But that's a minor thing.
OK, fine, will be fixed at the next version if no other objects.
Hi,
On 2024/3/8 03:37, Dmitry Baryshkov wrote:
> On Thu, 7 Mar 2024 at 21:20, Sui Jingfeng <[email protected]> wrote:
>> Hi,
>>
>>
>> On 2024/3/8 02:43, Dmitry Baryshkov wrote:
>>>> +
>>>> MODULE_AUTHOR("Ajay Kumar<[email protected]>");
>>>> MODULE_DESCRIPTION("DRM bridge infrastructure");
>>>> MODULE_LICENSE("GPL and additional rights");
>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>>> index 3606e1a7f965..d4c95afdd662 100644
>>>> --- a/include/drm/drm_bridge.h
>>>> +++ b/include/drm/drm_bridge.h
>>>> @@ -26,6 +26,7 @@
>>>> #include <linux/ctype.h>
>>>> #include <linux/list.h>
>>>> #include <linux/mutex.h>
>>>> +#include <linux/of.h>
>>>>
>>>> #include <drm/drm_atomic.h>
>>>> #include <drm/drm_encoder.h>
>>>> @@ -721,6 +722,8 @@ struct drm_bridge {
>>>> struct list_head chain_node;
>>>> /** @of_node: device node pointer to the bridge */
>>>> struct device_node *of_node;
>>> In my opinion, if you are adding fwnode, we can drop of_node
>>> completely. There is no need to keep both of them.
>>
>> But the 'struct device' have both of them contained, we should *follow the core*, right?
>> They are two major firmware subsystems (DT and ACPT), both are great and large, right?
>> Personally, I think the drm_bridge should embeds 'struct device', after all, drm bridge
>> are mainly stand for display bridges device. And also to reflect what you said: "to
>> reflect the hardware perfectly" and remove some boilerplate.
> struct device contains both because it is at the root of the hierarchy
> and it should support both API. drm_bridge is a consumer, so it's fine
> to have just one.
>
What I really means is that
if one day a 'struct device *dev' is embedded into struct drm_bridge,
we only need to improve(modify) the implement ofdrm_bridge_set_node().
This is *key point*. Currently, they(of_node and fwnode) point to the
same thing, it is also fine.
For the various drm bridge drivers implementations, the 'struct drm_bridge'
is also belong to the driver core category. drm bridges are also play the
producer role.
>> I think I'm not good enough to do such a big refactor, sorry. I believe that Maxime
>> and Laurent are the advanced programmers who is good enough to do such things, maybe
>> you can ask them for help?
> Well, you picked up the task ;-)
Well, my subject(title) is "*Allow* to use fwnode based API to get drm bridges".
not "Replace 'OF' with fwnode", My task is to provide an alternative solution
for the possible users. And to help improve code sharing and improve code reuse.
And remove some boilerplate. Others things beyond that is not being taken by
anybody. Thanks.
>
> But really, there is nothing so hard about it:
> - Change of_node to fw_node, apply an automatic patch changing this in
> bridge drivers.
> - Make drm_of_bridge functions convert passed of_node and comp
>
> After this we can start cleaning up bridge drivers to use fw_node API
> natively as you did in your patches 2-4.
Yes, it's not so hard. But I'm a little busy due to other downstream developing
tasks. Sorry, very sorry!
During the talk with you, I observed that you are very good at fwnode domain.
Are you willing to help the community to do something? For example, currently
the modern drm bridge framework is corrupted by legacy implement, is it possible
for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c
which create a drm connector manually, not modernized yet and it's DT dependent.
So, there are a lot things to do.
Best Regards,
Sui.
On Thu, 7 Mar 2024 at 22:32, Sui Jingfeng <[email protected]> wrote:
>
> Hi,
>
>
> On 2024/3/8 03:37, Dmitry Baryshkov wrote:
> > On Thu, 7 Mar 2024 at 21:20, Sui Jingfeng <[email protected]> wrote:
> >> Hi,
> >>
> >>
> >> On 2024/3/8 02:43, Dmitry Baryshkov wrote:
> >>>> +
> >>>> MODULE_AUTHOR("Ajay Kumar<[email protected]>");
> >>>> MODULE_DESCRIPTION("DRM bridge infrastructure");
> >>>> MODULE_LICENSE("GPL and additional rights");
> >>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> >>>> index 3606e1a7f965..d4c95afdd662 100644
> >>>> --- a/include/drm/drm_bridge.h
> >>>> +++ b/include/drm/drm_bridge.h
> >>>> @@ -26,6 +26,7 @@
> >>>> #include <linux/ctype.h>
> >>>> #include <linux/list.h>
> >>>> #include <linux/mutex.h>
> >>>> +#include <linux/of.h>
> >>>>
> >>>> #include <drm/drm_atomic.h>
> >>>> #include <drm/drm_encoder.h>
> >>>> @@ -721,6 +722,8 @@ struct drm_bridge {
> >>>> struct list_head chain_node;
> >>>> /** @of_node: device node pointer to the bridge */
> >>>> struct device_node *of_node;
> >>> In my opinion, if you are adding fwnode, we can drop of_node
> >>> completely. There is no need to keep both of them.
> >>
> >> But the 'struct device' have both of them contained, we should *follow the core*, right?
> >> They are two major firmware subsystems (DT and ACPT), both are great and large, right?
> >> Personally, I think the drm_bridge should embeds 'struct device', after all, drm bridge
> >> are mainly stand for display bridges device. And also to reflect what you said: "to
> >> reflect the hardware perfectly" and remove some boilerplate.
> > struct device contains both because it is at the root of the hierarchy
> > and it should support both API. drm_bridge is a consumer, so it's fine
> > to have just one.
> >
>
> What I really means is that
> if one day a 'struct device *dev' is embedded into struct drm_bridge,
> we only need to improve(modify) the implement ofdrm_bridge_set_node().
> This is *key point*. Currently, they(of_node and fwnode) point to the
> same thing, it is also fine.
>
> For the various drm bridge drivers implementations, the 'struct drm_bridge'
> is also belong to the driver core category. drm bridges are also play the
> producer role.
>
> >> I think I'm not good enough to do such a big refactor, sorry. I believe that Maxime
> >> and Laurent are the advanced programmers who is good enough to do such things, maybe
> >> you can ask them for help?
> > Well, you picked up the task ;-)
>
>
> Well, my subject(title) is "*Allow* to use fwnode based API to get drm bridges".
> not "Replace 'OF' with fwnode", My task is to provide an alternative solution
> for the possible users. And to help improve code sharing and improve code reuse.
> And remove some boilerplate. Others things beyond that is not being taken by
> anybody. Thanks.
Ok, I'd defer this to the maintainers decision then.
>
>
> >
> > But really, there is nothing so hard about it:
> > - Change of_node to fw_node, apply an automatic patch changing this in
> > bridge drivers.
> > - Make drm_of_bridge functions convert passed of_node and comp
> >
> > After this we can start cleaning up bridge drivers to use fw_node API
> > natively as you did in your patches 2-4.
>
>
> Yes, it's not so hard. But I'm a little busy due to other downstream developing
> tasks. Sorry, very sorry!
>
> During the talk with you, I observed that you are very good at fwnode domain.
> Are you willing to help the community to do something? For example, currently
> the modern drm bridge framework is corrupted by legacy implement, is it possible
> for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c
> which create a drm connector manually, not modernized yet and it's DT dependent.
> So, there are a lot things to do.
Actually, lontium-lt9611uxc.c does both of that ;-) It supports
creating a connector and it as well supports attaching to a chain
without creating a connector. Pretty nice, isn't it?
--
With best wishes
Dmitry
Hi,
On 2024/3/8 04:40, Dmitry Baryshkov wrote:
>>> But really, there is nothing so hard about it:
>>> - Change of_node to fw_node, apply an automatic patch changing this in
>>> bridge drivers.
>>> - Make drm_of_bridge functions convert passed of_node and comp
>>>
>>> After this we can start cleaning up bridge drivers to use fw_node API
>>> natively as you did in your patches 2-4.
>> Yes, it's not so hard. But I'm a little busy due to other downstream developing
>> tasks. Sorry, very sorry!
>>
>> During the talk with you, I observed that you are very good at fwnode domain.
>> Are you willing to help the community to do something? For example, currently
>> the modern drm bridge framework is corrupted by legacy implement, is it possible
>> for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c
>> which create a drm connector manually, not modernized yet and it's DT dependent.
>> So, there are a lot things to do.
> Actually, lontium-lt9611uxc.c does both of that ???? It supports
> creating a connector and it as well supports attaching to a chain
> without creating a connector. Pretty nice, isn't it?
I'm not very sure. But I remember a most experienced told me that
"Not having anything connector-related in the drm_bridge driver is a canonical design".
And I think he is right and I believed.
On 2024/3/8 05:09, Sui Jingfeng wrote:
> a most experienced
A most experienced programmer & developer.
Hi,
On 2024/3/8 04:40, Dmitry Baryshkov wrote:
>>> But really, there is nothing so hard about it:
>>> - Change of_node to fw_node, apply an automatic patch changing this in
>>> bridge drivers.
>>> - Make drm_of_bridge functions convert passed of_node and comp
>>>
>>> After this we can start cleaning up bridge drivers to use fw_node API
>>> natively as you did in your patches 2-4.
>> Yes, it's not so hard. But I'm a little busy due to other downstream developing
>> tasks. Sorry, very sorry!
>>
>> During the talk with you, I observed that you are very good at fwnode domain.
>> Are you willing to help the community to do something? For example, currently
>> the modern drm bridge framework is corrupted by legacy implement, is it possible
>> for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c
>> which create a drm connector manually, not modernized yet and it's DT dependent.
>> So, there are a lot things to do.
> Actually, lontium-lt9611uxc.c does both of that ???? It supports
> creating a connector and it as well supports attaching to a chain
> without creating a connector. Pretty nice, isn't it?
But why the drm_bridge_connector helpers and/or the drm_connector bridge can't suit you need?
Coding this way just add boilerplate into drm bridge subsystem, right?
The code path of "creating a connector" plus the code path of "not creating a connector"
forms a 'side-by-side' implementation imo.
Besides, I have repeated many times: the DT already speak everything.
Device drivers can completely know if there is a display connector OF device created and how many
display bridges in the whole chain. If there are connector device node in the DT, then it should
has a device driver bound to it(instead of create it manually) for a perfect implementation. As
you told me we should not *over play* the device-driver model, right?
On Sat, 9 Mar 2024 at 11:33, Sui Jingfeng <[email protected]> wrote:
>
> Hi,
>
>
> On 2024/3/8 04:40, Dmitry Baryshkov wrote:
> >>> But really, there is nothing so hard about it:
> >>> - Change of_node to fw_node, apply an automatic patch changing this in
> >>> bridge drivers.
> >>> - Make drm_of_bridge functions convert passed of_node and comp
> >>>
> >>> After this we can start cleaning up bridge drivers to use fw_node API
> >>> natively as you did in your patches 2-4.
> >> Yes, it's not so hard. But I'm a little busy due to other downstream developing
> >> tasks. Sorry, very sorry!
> >>
> >> During the talk with you, I observed that you are very good at fwnode domain.
> >> Are you willing to help the community to do something? For example, currently
> >> the modern drm bridge framework is corrupted by legacy implement, is it possible
> >> for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c
> >> which create a drm connector manually, not modernized yet and it's DT dependent.
> >> So, there are a lot things to do.
> > Actually, lontium-lt9611uxc.c does both of that ???? It supports
> > creating a connector and it as well supports attaching to a chain
> > without creating a connector. Pretty nice, isn't it?
>
>
> But why the drm_bridge_connector helpers and/or the drm_connector bridge can't suit you need?
> Coding this way just add boilerplate into drm bridge subsystem, right?
Because there are platforms, like iMX LCDIF which can use the
lt9611uxc bridge, but do not make use of the drm_bridge_connector yet.
> The code path of "creating a connector" plus the code path of "not creating a connector"
> forms a 'side-by-side' implementation imo.
>
> Besides, I have repeated many times: the DT already speak everything.
> Device drivers can completely know if there is a display connector OF device created and how many
> display bridges in the whole chain. If there are connector device node in the DT, then it should
> has a device driver bound to it(instead of create it manually) for a perfect implementation. As
> you told me we should not *over play* the device-driver model, right?
Please, don't mix the connector node in DT and the drm_connector. If
you check the code, you will see that the driver for hdmi-connector,
dp-connector and other such devices creates a drm_bridge.
--
With best wishes
Dmitry
Hi,
On 2024/3/9 18:39, Dmitry Baryshkov wrote:
> On Sat, 9 Mar 2024 at 11:33, Sui Jingfeng <[email protected]> wrote:
>> Hi,
>>
>>
>> On 2024/3/8 04:40, Dmitry Baryshkov wrote:
>>>>> But really, there is nothing so hard about it:
>>>>> - Change of_node to fw_node, apply an automatic patch changing this in
>>>>> bridge drivers.
>>>>> - Make drm_of_bridge functions convert passed of_node and comp
>>>>>
>>>>> After this we can start cleaning up bridge drivers to use fw_node API
>>>>> natively as you did in your patches 2-4.
>>>> Yes, it's not so hard. But I'm a little busy due to other downstream developing
>>>> tasks. Sorry, very sorry!
>>>>
>>>> During the talk with you, I observed that you are very good at fwnode domain.
>>>> Are you willing to help the community to do something? For example, currently
>>>> the modern drm bridge framework is corrupted by legacy implement, is it possible
>>>> for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c
>>>> which create a drm connector manually, not modernized yet and it's DT dependent.
>>>> So, there are a lot things to do.
>>> Actually, lontium-lt9611uxc.c does both of that ???? It supports
>>> creating a connector and it as well supports attaching to a chain
>>> without creating a connector. Pretty nice, isn't it?
>>
>> But why the drm_bridge_connector helpers and/or the drm_connector bridge can't suit you need?
>> Coding this way just add boilerplate into drm bridge subsystem, right?
> Because there are platforms, like iMX LCDIF which can use the
> lt9611uxc bridge, but do not make use of the drm_bridge_connector yet.
>
Well, I have just grepped across the drm-tip kernel branch, but I don't find
iMX LCDIF you mentioned. See the search results pasted at bellow.
$ find . -name "*.dts" -type f | xargs grep "lontium,lt9611uxc"
/arm64/boot/dts/qcom/sm8450-hdk.dts: compatible = "lontium,lt9611uxc";
/arm64/boot/dts/qcom/qrb5165-rb5.dts: compatible = "lontium,lt9611uxc";
/arm64/boot/dts/qcom/qrb2210-rb1.dts: compatible = "lontium,lt9611uxc";
/arm64/boot/dts/qcom/qrb4210-rb2.dts: compatible = "lontium,lt9611uxc";
/arm64/boot/dts/qcom/sm8350-hdk.dts: compatible = "lontium,lt9611uxc";
So I can't see the drm driver that you refer to, can you pointed it out for study
purpose? Even it's exist, however, back to that time, why don't you posting a patch
to switch it to the canonical design as you mentioned and give the community a clean
design?
And those are just *reasons*, from the viewpoint of the *result*.
The merged patch results in a 'side-by-side' implement and boilerplate added
into drm bridges subsystem, the results doesn't change no matter what the
reason is, right?
Hi,
On 2024/3/9 18:39, Dmitry Baryshkov wrote:
>> The code path of "creating a connector" plus the code path of "not creating a connector"
>> forms a 'side-by-side' implementation imo.
>>
>> Besides, I have repeated many times: the DT already speak everything.
>> Device drivers can completely know if there is a display connector OF device created and how many
>> display bridges in the whole chain. If there are connector device node in the DT, then it should
>> has a device driver bound to it(instead of create it manually) for a perfect implementation. As
>> you told me we should not*over play* the device-driver model, right?
> Please, don't mix the connector node in DT and the drm_connector. If
> you check the code, you will see that the driver for hdmi-connector,
> dp-connector and other such devices creates a drm_bridge.
OK, I'm not mixed them, I'm very clear from the very beginning. I have checked
the code years ago. Let's make it clear by iterating one more time:
If DT contains one or more HDMI connector node, then there will be one or
more display connector platform devices created by OF core, Then, according to
your "don't overplay device-driver model" criterion or modern drm bridge standard,
we shouldn't create a display connector instance in the drm birdge driver, right?
As otherwise we will have two display connector driver (or code) for a single entity,
right?
Another side effect is that
when you create a hdmi display connector instance manually without reference to the
DT, then *you made an assumption!*. But there are users who have don't a different
need(or different typology), for example, they need to read edid directly from the
KMS driver side. This may because the i2c bus is directly connected to the connector
part, but the display bridge's I2C slave interface. sii9022, it66121 and tfp410 support
this kind of usage.
So the real problem is that it is a policy level code when you creating a hdmi
display connector instance manually without reference to the DT in a common drm bridge
driver, not a mechanism.
On Sat, 9 Mar 2024 at 13:25, Sui Jingfeng <[email protected]> wrote:
>
> Hi,
>
>
> On 2024/3/9 18:39, Dmitry Baryshkov wrote:
> > On Sat, 9 Mar 2024 at 11:33, Sui Jingfeng <[email protected]> wrote:
> >> Hi,
> >>
> >>
> >> On 2024/3/8 04:40, Dmitry Baryshkov wrote:
> >>>>> But really, there is nothing so hard about it:
> >>>>> - Change of_node to fw_node, apply an automatic patch changing this in
> >>>>> bridge drivers.
> >>>>> - Make drm_of_bridge functions convert passed of_node and comp
> >>>>>
> >>>>> After this we can start cleaning up bridge drivers to use fw_node API
> >>>>> natively as you did in your patches 2-4.
> >>>> Yes, it's not so hard. But I'm a little busy due to other downstream developing
> >>>> tasks. Sorry, very sorry!
> >>>>
> >>>> During the talk with you, I observed that you are very good at fwnode domain.
> >>>> Are you willing to help the community to do something? For example, currently
> >>>> the modern drm bridge framework is corrupted by legacy implement, is it possible
> >>>> for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c
> >>>> which create a drm connector manually, not modernized yet and it's DT dependent.
> >>>> So, there are a lot things to do.
> >>> Actually, lontium-lt9611uxc.c does both of that ???? It supports
> >>> creating a connector and it as well supports attaching to a chain
> >>> without creating a connector. Pretty nice, isn't it?
> >>
> >> But why the drm_bridge_connector helpers and/or the drm_connector bridge can't suit you need?
> >> Coding this way just add boilerplate into drm bridge subsystem, right?
> > Because there are platforms, like iMX LCDIF which can use the
> > lt9611uxc bridge, but do not make use of the drm_bridge_connector yet.
> >
>
> Well, I have just grepped across the drm-tip kernel branch, but I don't find
> iMX LCDIF you mentioned. See the search results pasted at bellow.
Please take a look at the commit 8ddce13ae696 ("drm/bridge: lt9611: Do
not generate HFP/HBP/HSA and EOT packet"). As you can see from the
description, Marek has been using this bridge with th iMX8MM / iMX8MP
boards. As soon as mxsfb has been updated to pass
DRM_BRIDGE_ATTACH_NO_CONNECTOR, we can drop corresponding code from
LT9611UXC driver.
> $ find . -name "*.dts" -type f | xargs grep "lontium,lt9611uxc"
> ./arm64/boot/dts/qcom/sm8450-hdk.dts: compatible = "lontium,lt9611uxc";
> ./arm64/boot/dts/qcom/qrb5165-rb5.dts: compatible = "lontium,lt9611uxc";
> ./arm64/boot/dts/qcom/qrb2210-rb1.dts: compatible = "lontium,lt9611uxc";
> ./arm64/boot/dts/qcom/qrb4210-rb2.dts: compatible = "lontium,lt9611uxc";
> ./arm64/boot/dts/qcom/sm8350-hdk.dts: compatible = "lontium,lt9611uxc";
>
>
> So I can't see the drm driver that you refer to, can you pointed it out for study
> purpose? Even it's exist, however, back to that time, why don't you posting a patch
> to switch it to the canonical design as you mentioned and give the community a clean
> design?
If you have iMX8 hardware, please do it. I don't have these boards and
I do not have an intention of acquiring them.
> And those are just *reasons*, from the viewpoint of the *result*.
> The merged patch results in a 'side-by-side' implement and boilerplate added
> into drm bridges subsystem, the results doesn't change no matter what the
> reason is, right?
--
With best wishes
Dmitry
On Sat, 9 Mar 2024 at 14:03, Sui Jingfeng <[email protected]> wrote:
>
> Hi,
>
>
> On 2024/3/9 18:39, Dmitry Baryshkov wrote:
> >> The code path of "creating a connector" plus the code path of "not creating a connector"
> >> forms a 'side-by-side' implementation imo.
> >>
> >> Besides, I have repeated many times: the DT already speak everything.
> >> Device drivers can completely know if there is a display connector OF device created and how many
> >> display bridges in the whole chain. If there are connector device node in the DT, then it should
> >> has a device driver bound to it(instead of create it manually) for a perfect implementation. As
> >> you told me we should not*over play* the device-driver model, right?
> > Please, don't mix the connector node in DT and the drm_connector. If
> > you check the code, you will see that the driver for hdmi-connector,
> > dp-connector and other such devices creates a drm_bridge.
>
>
> OK, I'm not mixed them, I'm very clear from the very beginning. I have checked
> the code years ago. Let's make it clear by iterating one more time:
>
> If DT contains one or more HDMI connector node, then there will be one or
> more display connector platform devices created by OF core, Then, according to
> your "don't overplay device-driver model" criterion or modern drm bridge standard,
> we shouldn't create a display connector instance in the drm birdge driver, right?
Yeah, if the platform is updated, yes, we do. If there is an
hdmi-connector node, I can only assume that the DRM driver also has
been updated to pass the DRM_BRIDGE_ATTACH_NO_CONNECTOR. In such case
the lt9611uxc driver will not create the drm_connector and everything
works as expected. If this is one of the legacy platforms, the DRM
driver will not pass the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, but at
the same time the DT will not have the connector node.
>
> As otherwise we will have two display connector driver (or code) for a single entity,
> right?
>
> Another side effect is that
> when you create a hdmi display connector instance manually without reference to the
> DT, then *you made an assumption!*. But there are users who have don't a different
> need(or different typology), for example, they need to read edid directly from the
> KMS driver side. This may because the i2c bus is directly connected to the connector
> part, but the display bridge's I2C slave interface. sii9022, it66121 and tfp410 support
> this kind of usage.
>
> So the real problem is that it is a policy level code when you creating a hdmi
> display connector instance manually without reference to the DT in a common drm bridge
> driver, not a mechanism.
Only if requested by the DRM driver itself.
--
With best wishes
Dmitry