usb: typec: nb7vpq904m: add CONFIG_DRM dependency

[PATCH] usb: typec: nb7vpq904m: add CONFIG_DRM dependency

Posted by Arnd Bergmann 2 years, 7 months ago

From: Arnd Bergmann <arnd@arndb.de>

This driver calls directly into DRM functions and fails to link if
that is disabled:

ld.lld: error: undefined symbol: devm_drm_bridge_add
ld.lld: error: undefined symbol: devm_drm_of_get_bridge
>>> referenced by nb7vpq904m.c
>>>               drivers/usb/typec/mux/nb7vpq904m.o:(nb7vpq904m_probe) in archive vmlinux.a

Fixes: 88d8f3ac9c67e ("usb: typec: add support for the nb7vpq904m Type-C Linear Redriver")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/usb/typec/mux/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
index 8c4d6b8fb75c3..f53ae24b6c048 100644
--- a/drivers/usb/typec/mux/Kconfig
+++ b/drivers/usb/typec/mux/Kconfig
@@ -37,7 +37,7 @@ config TYPEC_MUX_INTEL_PMC
 
 config TYPEC_MUX_NB7VPQ904M
 	tristate "On Semiconductor NB7VPQ904M Type-C redriver driver"
-	depends on I2C
+	depends on I2C && DRM
 	select REGMAP_I2C
 	help
 	  Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C
-- 
2.39.2

Re: [PATCH] usb: typec: nb7vpq904m: add CONFIG_DRM dependency

Posted by Neil Armstrong 2 years, 7 months ago

Hi,

On 22/06/2023 12:18, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> This driver calls directly into DRM functions and fails to link if
> that is disabled:
> 
> ld.lld: error: undefined symbol: devm_drm_bridge_add
> ld.lld: error: undefined symbol: devm_drm_of_get_bridge
>>>> referenced by nb7vpq904m.c
>>>>                drivers/usb/typec/mux/nb7vpq904m.o:(nb7vpq904m_probe) in archive vmlinux.a
> 
> Fixes: 88d8f3ac9c67e ("usb: typec: add support for the nb7vpq904m Type-C Linear Redriver")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   drivers/usb/typec/mux/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
> index 8c4d6b8fb75c3..f53ae24b6c048 100644
> --- a/drivers/usb/typec/mux/Kconfig
> +++ b/drivers/usb/typec/mux/Kconfig
> @@ -37,7 +37,7 @@ config TYPEC_MUX_INTEL_PMC
>   
>   config TYPEC_MUX_NB7VPQ904M
>   	tristate "On Semiconductor NB7VPQ904M Type-C redriver driver"
> -	depends on I2C
> +	depends on I2C && DRM
>   	select REGMAP_I2C
>   	help
>   	  Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C

I think it could be :

+	depends on DRM || DRM=n
+	select DRM_PANEL_BRIDGE if DRM

Neil

Re: [PATCH] usb: typec: nb7vpq904m: add CONFIG_DRM dependency

Posted by Arnd Bergmann 2 years, 7 months ago

On Thu, Jun 22, 2023, at 12:39, Neil Armstrong wrote:
> Hi,
>
> On 22/06/2023 12:18, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> This driver calls directly into DRM functions and fails to link if
>> that is disabled:
>> 
>> ld.lld: error: undefined symbol: devm_drm_bridge_add
>> ld.lld: error: undefined symbol: devm_drm_of_get_bridge
>>>>> referenced by nb7vpq904m.c
>>>>>                drivers/usb/typec/mux/nb7vpq904m.o:(nb7vpq904m_probe) in archive vmlinux.a
>> 
>> Fixes: 88d8f3ac9c67e ("usb: typec: add support for the nb7vpq904m Type-C Linear Redriver")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>   drivers/usb/typec/mux/Kconfig | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
>> index 8c4d6b8fb75c3..f53ae24b6c048 100644
>> --- a/drivers/usb/typec/mux/Kconfig
>> +++ b/drivers/usb/typec/mux/Kconfig
>> @@ -37,7 +37,7 @@ config TYPEC_MUX_INTEL_PMC
>>   
>>   config TYPEC_MUX_NB7VPQ904M
>>   	tristate "On Semiconductor NB7VPQ904M Type-C redriver driver"
>> -	depends on I2C
>> +	depends on I2C && DRM
>>   	select REGMAP_I2C
>>   	help
>>   	  Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C
>
> I think it could be :
>
> +	depends on DRM || DRM=n
> +	select DRM_PANEL_BRIDGE if DRM

As far as I can tell, this would only avoid the link error
against devm_drm_of_get_bridge(), but not the one against
devm_drm_bridge_add(), which is defined in drm.ko through
drivers/gpu/drm/drm_bridge.c.

    Arnd

Re: [PATCH] usb: typec: nb7vpq904m: add CONFIG_DRM dependency

Posted by Neil Armstrong 2 years, 7 months ago

Hi,

On 22/06/2023 13:24, Arnd Bergmann wrote:
> On Thu, Jun 22, 2023, at 12:39, Neil Armstrong wrote:
>> Hi,
>>
>> On 22/06/2023 12:18, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> This driver calls directly into DRM functions and fails to link if
>>> that is disabled:
>>>
>>> ld.lld: error: undefined symbol: devm_drm_bridge_add
>>> ld.lld: error: undefined symbol: devm_drm_of_get_bridge
>>>>>> referenced by nb7vpq904m.c
>>>>>>                 drivers/usb/typec/mux/nb7vpq904m.o:(nb7vpq904m_probe) in archive vmlinux.a
>>>
>>> Fixes: 88d8f3ac9c67e ("usb: typec: add support for the nb7vpq904m Type-C Linear Redriver")
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> ---
>>>    drivers/usb/typec/mux/Kconfig | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
>>> index 8c4d6b8fb75c3..f53ae24b6c048 100644
>>> --- a/drivers/usb/typec/mux/Kconfig
>>> +++ b/drivers/usb/typec/mux/Kconfig
>>> @@ -37,7 +37,7 @@ config TYPEC_MUX_INTEL_PMC
>>>    
>>>    config TYPEC_MUX_NB7VPQ904M
>>>    	tristate "On Semiconductor NB7VPQ904M Type-C redriver driver"
>>> -	depends on I2C
>>> +	depends on I2C && DRM
>>>    	select REGMAP_I2C
>>>    	help
>>>    	  Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C
>>
>> I think it could be :
>>
>> +	depends on DRM || DRM=n
>> +	select DRM_PANEL_BRIDGE if DRM
> 
> As far as I can tell, this would only avoid the link error
> against devm_drm_of_get_bridge(), but not the one against
> devm_drm_bridge_add(), which is defined in drm.ko through
> drivers/gpu/drm/drm_bridge.c.

I'm trying to reproduce such situation, but so fail I fail.

In the driver there's a guard to avoid calling into DRM functions
when disabled:
#if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_DRM_PANEL_BRIDGE)

so I wonder which kind on config leaded to that since
CONFIG_DRM_PANEL_BRIDGE is only enabled when DRM_PANEL and DRM are.

Neil

> 
>      Arnd

Re: [PATCH] usb: typec: nb7vpq904m: add CONFIG_DRM dependency

Posted by Arnd Bergmann 2 years, 7 months ago

On Thu, Jun 22, 2023, at 17:39, Neil Armstrong wrote:
> On 22/06/2023 13:24, Arnd Bergmann wrote:
>> On Thu, Jun 22, 2023, at 12:39, Neil Armstrong wrote:
>>>>    
>>>>    config TYPEC_MUX_NB7VPQ904M
>>>>    	tristate "On Semiconductor NB7VPQ904M Type-C redriver driver"
>>>> -	depends on I2C
>>>> +	depends on I2C && DRM
>>>>    	select REGMAP_I2C
>>>>    	help
>>>>    	  Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C
>>>
>>> I think it could be :
>>>
>>> +	depends on DRM || DRM=n
>>> +	select DRM_PANEL_BRIDGE if DRM
>> 
>> As far as I can tell, this would only avoid the link error
>> against devm_drm_of_get_bridge(), but not the one against
>> devm_drm_bridge_add(), which is defined in drm.ko through
>> drivers/gpu/drm/drm_bridge.c.
>
> I'm trying to reproduce such situation, but so fail I fail.
>
> In the driver there's a guard to avoid calling into DRM functions
> when disabled:
> #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_DRM_PANEL_BRIDGE)

Ah, you are right. I got confused because the check is in
header file for devm_drm_of_get_bridge(), but not for
devm_drm_bridge_add(), which has the check in the source
file as you point out.

> so I wonder which kind on config leaded to that since
> CONFIG_DRM_PANEL_BRIDGE is only enabled when DRM_PANEL and DRM are.

I only saw the original issue with

CONFIG_DRM=m
CONFIG_DRM_PANEL=y
CONFIG_DRM_BRIDGE=y
CONFIG_DRM_PANEL_BRIDGE=y
CONFIG_TYPEC_MUX_NB7VPQ904M=y

and since CONFIG_DRM_PANEL_BRIDGE already depends on CONFIG_DRM,
I think that is the only one that can go wrong, so your
suggestion of

   depends on DRM || DRM=n

should be sufficient. I see that DRM_PANEL, DRM_BRIDGE and
DRM_PANEL_BRIDGE are now always =y whenever DRM is enabled,
so I don't think the 'select CONFIG_DRM_PANEL_BRIDGE' serves
any purpose any more, but it's also harmless if you think it
helps for clarification.

Can you send the updated patch, or should I?


     Arnd

Re: [PATCH] usb: typec: nb7vpq904m: add CONFIG_DRM dependency

Posted by Neil Armstrong 2 years, 7 months ago

On 22/06/2023 17:57, Arnd Bergmann wrote:
> On Thu, Jun 22, 2023, at 17:39, Neil Armstrong wrote:
>> On 22/06/2023 13:24, Arnd Bergmann wrote:
>>> On Thu, Jun 22, 2023, at 12:39, Neil Armstrong wrote:
>>>>>     
>>>>>     config TYPEC_MUX_NB7VPQ904M
>>>>>     	tristate "On Semiconductor NB7VPQ904M Type-C redriver driver"
>>>>> -	depends on I2C
>>>>> +	depends on I2C && DRM
>>>>>     	select REGMAP_I2C
>>>>>     	help
>>>>>     	  Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C
>>>>
>>>> I think it could be :
>>>>
>>>> +	depends on DRM || DRM=n
>>>> +	select DRM_PANEL_BRIDGE if DRM
>>>
>>> As far as I can tell, this would only avoid the link error
>>> against devm_drm_of_get_bridge(), but not the one against
>>> devm_drm_bridge_add(), which is defined in drm.ko through
>>> drivers/gpu/drm/drm_bridge.c.
>>
>> I'm trying to reproduce such situation, but so fail I fail.
>>
>> In the driver there's a guard to avoid calling into DRM functions
>> when disabled:
>> #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_DRM_PANEL_BRIDGE)
> 
> Ah, you are right. I got confused because the check is in
> header file for devm_drm_of_get_bridge(), but not for
> devm_drm_bridge_add(), which has the check in the source
> file as you point out.
> 
>> so I wonder which kind on config leaded to that since
>> CONFIG_DRM_PANEL_BRIDGE is only enabled when DRM_PANEL and DRM are.
> 
> I only saw the original issue with
> 
> CONFIG_DRM=m
> CONFIG_DRM_PANEL=y
> CONFIG_DRM_BRIDGE=y
> CONFIG_DRM_PANEL_BRIDGE=y
> CONFIG_TYPEC_MUX_NB7VPQ904M=y

Ok now I see it

> 
> and since CONFIG_DRM_PANEL_BRIDGE already depends on CONFIG_DRM,
> I think that is the only one that can go wrong, so your
> suggestion of
> 
>     depends on DRM || DRM=n
> 
> should be sufficient. I see that DRM_PANEL, DRM_BRIDGE and
> DRM_PANEL_BRIDGE are now always =y whenever DRM is enabled,
> so I don't think the 'select CONFIG_DRM_PANEL_BRIDGE' serves
> any purpose any more, but it's also harmless if you think it
> helps for clarification.

Yes DRM || DRM=n seems sufficient, but having
select DRM_PANEL_BRIDGE if DRM could clarify things.

> 
> Can you send the updated patch, or should I?

I'll send an updated patch,

Thanks,
Neil

> 
> 
>       Arnd