2023-06-22 10:55:16

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH] usb: typec: nb7vpq904m: add CONFIG_DRM dependency

From: Arnd Bergmann <[email protected]>

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 <[email protected]>
---
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



2023-06-22 11:14:48

by Neil Armstrong

[permalink] [raw]
Subject: Re: [PATCH] usb: typec: nb7vpq904m: add CONFIG_DRM dependency

Hi,

On 22/06/2023 12:18, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> 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 <[email protected]>
> ---
> 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

2023-06-22 11:41:41

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] usb: typec: nb7vpq904m: add CONFIG_DRM dependency

On Thu, Jun 22, 2023, at 12:39, Neil Armstrong wrote:
> Hi,
>
> On 22/06/2023 12:18, Arnd Bergmann wrote:
>> From: Arnd Bergmann <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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

2023-06-22 16:09:57

by Neil Armstrong

[permalink] [raw]
Subject: Re: [PATCH] usb: typec: nb7vpq904m: add CONFIG_DRM dependency

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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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


2023-06-22 16:13:51

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] usb: typec: nb7vpq904m: add CONFIG_DRM dependency

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

2023-06-22 16:20:44

by Neil Armstrong

[permalink] [raw]
Subject: Re: [PATCH] usb: typec: nb7vpq904m: add CONFIG_DRM dependency

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