Received-SPF: pass (google.com: domain of linux-kernel+bounces-179714-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Jani Nikula <jani.nikula@linux.intel.com>
To: Sui Jingfeng <sui.jingfeng@linux.dev>, Neil Armstrong
 <neil.armstrong@linaro.org>, Maxime Ripard <mripard@kernel.org>, Dmitry
 Baryshkov <dmitry.baryshkov@linaro.org>
Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Sui
 Jingfeng <sui.jingfeng@linux.dev>
Subject: Re: [PATCH 1/2] drm/bridge: Support finding bridge with struct device
In-Reply-To: <20240514154045.309925-2-sui.jingfeng@linux.dev>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
References: <20240514154045.309925-1-sui.jingfeng@linux.dev>
 <20240514154045.309925-2-sui.jingfeng@linux.dev>
Date: Wed, 15 May 2024 12:39:37 +0300
Message-ID: <87v83fct2e.fsf@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Tue, 14 May 2024, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> The pointer of 'struct device' can also be used as a key to search drm
> bridge instance from the global bridge list, traditionally, fwnode and
> 'OF' based APIs requires the system has decent fwnode/OF Graph support.
> While the drm_find_bridge_by_dev() function introduced in this series
> don't has such a restriction. It only require you has a pointer to the
> backing device. Hence, it may suitable for some small and/or limited
> display subsystems.
>
> Also add the drm_bridge_add_with_dev() as a helper, which automatically
> set the .of_node field of drm_bridge instances if you call it. But it
> suitable for simple bridge drivers which one device backing one drm_bridge
> instance.
>
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/drm_bridge.c | 39 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     |  5 +++++
>  2 files changed, 44 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 584d109330ab..1928d9d0dd3c 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -213,6 +213,23 @@ void drm_bridge_add(struct drm_bridge *bridge)
>  }
>  EXPORT_SYMBOL(drm_bridge_add);
>  
> +/**
> + * drm_bridge_add_with_dev - add the given bridge to the global bridge list
> + *
> + * @bridge: bridge control structure
> + * @dev: pointer to the kernel device that this bridge is backed.
> + */
> +void drm_bridge_add_with_dev(struct drm_bridge *bridge, struct device *dev)
> +{
> +	if (dev) {
> +		bridge->kdev = dev;
> +		bridge->of_node = dev->of_node;
> +	}
> +
> +	drm_bridge_add(bridge);
> +}
> +EXPORT_SYMBOL_GPL(drm_bridge_add_with_dev);

I don't actually have an opinion on whether the dev parameter is useful
or not.

But please don't add a drm_bridge_add_with_dev() and then convert more
than half the drm_bridge_add() users to that. Please just add a struct
device *dev parameter to drm_bridge_add(), and pass NULL if it's not
relevant.

BR,
Jani.


> +
>  static void drm_bridge_remove_void(void *bridge)
>  {
>  	drm_bridge_remove(bridge);
> @@ -1334,6 +1351,27 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
>  }
>  EXPORT_SYMBOL_GPL(drm_bridge_hpd_notify);
>  
> +struct drm_bridge *drm_find_bridge_by_dev(struct device *kdev)
> +{
> +	struct drm_bridge *bridge;
> +
> +	if (!kdev)
> +		return NULL;
> +
> +	mutex_lock(&bridge_lock);
> +
> +	list_for_each_entry(bridge, &bridge_list, list) {
> +		if (bridge->kdev == kdev) {
> +			mutex_unlock(&bridge_lock);
> +			return bridge;
> +		}
> +	}
> +
> +	mutex_unlock(&bridge_lock);
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(drm_find_bridge_by_dev);
> +
>  #ifdef CONFIG_OF
>  /**
>   * of_drm_find_bridge - find the bridge corresponding to the device node in
> @@ -1361,6 +1399,7 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>  	return NULL;
>  }
>  EXPORT_SYMBOL(of_drm_find_bridge);
> +
>  #endif
>  
>  MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 4baca0d9107b..70d8393bbd9c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -715,6 +715,8 @@ struct drm_bridge {
>  	struct drm_private_obj base;
>  	/** @dev: DRM device this bridge belongs to */
>  	struct drm_device *dev;
> +	/** @kdev: pointer to the kernel device backing this bridge */
> +	struct device *kdev;
>  	/** @encoder: encoder to which this bridge is connected */
>  	struct drm_encoder *encoder;
>  	/** @chain_node: used to form a bridge chain */
> @@ -782,12 +784,15 @@ drm_priv_to_bridge(struct drm_private_obj *priv)
>  }
>  
>  void drm_bridge_add(struct drm_bridge *bridge);
> +void drm_bridge_add_with_dev(struct drm_bridge *bridge, struct device *dev);
>  int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge);
>  void drm_bridge_remove(struct drm_bridge *bridge);
>  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  		      struct drm_bridge *previous,
>  		      enum drm_bridge_attach_flags flags);
>  
> +struct drm_bridge *drm_find_bridge_by_dev(struct device *kdev);
> +
>  #ifdef CONFIG_OF
>  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
>  #else

-- 
Jani Nikula, Intel