Received-SPF: pass (google.com: domain of linux-kernel+bounces-179761-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
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
Subject: Re: [PATCH 1/2] drm/bridge: Support finding bridge with struct device
In-Reply-To: <cd81893c-ef0b-4906-8c9c-a98b1e4669e6@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>
 <87v83fct2e.fsf@intel.com>
 <cd81893c-ef0b-4906-8c9c-a98b1e4669e6@linux.dev>
Date: Wed, 15 May 2024 13:28:11 +0300
Message-ID: <87pltncqtg.fsf@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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