Re: [PATCH 1/2] drm/bridge: Support finding bridge with struct device

[Sui Jingfeng <sui.jingfeng@linux.dev>]

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