Re: [PATCH v2 1/4] drm/bridge: Add fwnode based helpers to get the next bridge

Dmitry Baryshkov
2024-03-07T18:43:27Z

On Thu, 7 Mar 2024 at 19:23, Sui Jingfeng <sui.jingfeng@linux.dev> 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 <sui.jingfeng@linux.dev>
> ---
> 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 <ajaykumar.rs@samsung.com>");
> 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