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);
+
 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
-- 
2.43.0

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

Hi,


On 5/15/24 17:39, Jani Nikula wrote:
> 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.
> 

To be honest, previously, I'm just do it exactly same as the way you
told me here. But I'm exhausted and finally give up.

Because this is again need me to modify *all* callers of 
drm_bridge_add(), not only those bridges in drm/bridge/, but also
bridge instances in various KMS drivers.

However, their some exceptions just don't fit!

For example, the imx/imx8qxp-pixel-combiner.c just don't fit our
simple model. Our helper function assume that one device backing
one drm_bridge instance (1 to 1). Yet, that driver backing two or
more bridges with one platform device (1 to 2, 1 to 3, ..., ).
Hence, the imx/imx8qxp-pixel-combiner.c just can't use 
drm_bridge_add_with_dev().

The aux_hpd_bridge.c is also bad, it store the of_node of struct device 
at the .platform_data member of the struct device.

> 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
> 

-- 
Best regards
Sui

On Wed, 15 May 2024, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> Hi,
>
>
> On 5/15/24 17:39, Jani Nikula wrote:
>> On Tue, 14 May 2024, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>> 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.
>> 
>
> To be honest, previously, I'm just do it exactly same as the way you
> told me here. But I'm exhausted and finally give up.
>
> Because this is again need me to modify *all* callers of 
> drm_bridge_add(), not only those bridges in drm/bridge/, but also
> bridge instances in various KMS drivers.
>
> However, their some exceptions just don't fit!
>
> For example, the imx/imx8qxp-pixel-combiner.c just don't fit our
> simple model. Our helper function assume that one device backing
> one drm_bridge instance (1 to 1). Yet, that driver backing two or
> more bridges with one platform device (1 to 2, 1 to 3, ..., ).
> Hence, the imx/imx8qxp-pixel-combiner.c just can't use 
> drm_bridge_add_with_dev().
>
> The aux_hpd_bridge.c is also bad, it store the of_node of struct device 
> at the .platform_data member of the struct device.

Like I said, "pass NULL if it's not relevant."

"_add_with_dev" is a terrible function name.

What if you need to add another parameter later? Add _add_with_foo and
_add_with_dev_and_foo variants?

BR,
Jani.


-- 
Jani Nikula, Intel

Hi,


On 5/15/24 18:28, Jani Nikula wrote:
> On Wed, 15 May 2024, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>> Hi,
>>
>>
>> On 5/15/24 17:39, Jani Nikula wrote:
>>> On Tue, 14 May 2024, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>>> 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.
>>>
>>
>> To be honest, previously, I'm just do it exactly same as the way you
>> told me here. But I'm exhausted and finally give up.
>>
>> Because this is again need me to modify *all* callers of
>> drm_bridge_add(), not only those bridges in drm/bridge/, but also
>> bridge instances in various KMS drivers.
>>
>> However, their some exceptions just don't fit!
>>
>> For example, the imx/imx8qxp-pixel-combiner.c just don't fit our
>> simple model. Our helper function assume that one device backing
>> one drm_bridge instance (1 to 1). Yet, that driver backing two or
>> more bridges with one platform device (1 to 2, 1 to 3, ..., ).
>> Hence, the imx/imx8qxp-pixel-combiner.c just can't use
>> drm_bridge_add_with_dev().
>>
>> The aux_hpd_bridge.c is also bad, it store the of_node of struct device
>> at the .platform_data member of the struct device.
> 
> Like I said, "pass NULL if it's not relevant."

OK, good idea.

> "_add_with_dev" is a terrible function name.
> 
> What if you need to add another parameter later? Add _add_with_foo and
> _add_with_dev_and_foo variants?

Yes, you are right, I'll back give another try then.

> BR,
> Jani.
> 
> 

-- 
Best regards
Sui

On Wed, 15 May 2024, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
> Yes, you are right, I'll back give another try then.

Thanks, but please do wait until you have feedback on whether the whole
thing is a good idea or not. :)

BR,
Jani.


-- 
Jani Nikula, Intel