Re: [PATCH 1/2] fbdev: don't select I2C directly

From: Randy Dunlap
Date: Thu Feb 01 2018 - 19:21:31 EST

On 02/01/2018 08:14 AM, Bartlomiej Zolnierkiewicz wrote:
> On Monday, January 15, 2018 05:14:04 PM Arnd Bergmann wrote:
>> Using a Kconfig 'select' statement for a user-visible symbol that other
>> drivers depend on often causes circular dependencies. A new one showed
>> up when I wanted to add an NVMEM dependency to the DRM_MSM driver:
>>
>> drivers/i2c/Kconfig:7:error: recursive dependency detected!
>> drivers/i2c/Kconfig:7: symbol I2C is selected by FB_DDC
>> drivers/video/fbdev/Kconfig:63: symbol FB_DDC is selected by FB_CYBER2000_DDC
>> drivers/video/fbdev/Kconfig:390: symbol FB_CYBER2000_DDC depends on FB_CYBER2000
>> drivers/video/fbdev/Kconfig:378: symbol FB_CYBER2000 depends on FB
>> drivers/video/fbdev/Kconfig:5: symbol FB is selected by DRM_KMS_FB_HELPER
>> drivers/gpu/drm/Kconfig:77: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
>> drivers/gpu/drm/Kconfig:71: symbol DRM_KMS_HELPER is selected by DRM_MSM
>> drivers/gpu/drm/msm/Kconfig:2: symbol DRM_MSM depends on NVMEM
>> drivers/nvmem/Kconfig:1: symbol NVMEM is selected by EEPROM_AT24
>> drivers/misc/eeprom/Kconfig:3: symbol EEPROM_AT24 depends on I2C
>>
>> Here, the problem is that many fbdev drivers have an i2c based interface
>> and just 'select i2c' for that, while we also select the framebuffer
>> subsystem indirectly from a DRM driver that now depends on i2c.
>>
>> This does away with the 'select' statement and instead uses 'depends on',
>> like almost all I2C users.
>
> I worry that this change may cause various driver options to no longer
> be visible to people configuring their kernels and not having I2C already
> selected.
>
> DRM somehow manages to select I2C and I would prefer to be it the same
> way for fbdev (also looking at the current next tree there are still
> some drivers that 'select I2C')..

a. Linus has stated that a driver should not enable an entire subsystem,
so depends on would be better than select.
If I had the email/patch, I would be glad to Ack it.

b. DRM configuration is a mess. You shouldn't want to follow their model. :)

c. If I2C is not enabled in the FB menu, someone could just add something like this:

comment "Enable I2C to see more driver choices"
depends on !I2C


>> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
>> ---
>> drivers/video/fbdev/Kconfig | 19 +++++++++++++++----
>> 1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
>> index 6962b4583fd7..892eb1863100 100644
>> --- a/drivers/video/fbdev/Kconfig
>> +++ b/drivers/video/fbdev/Kconfig
>> @@ -64,7 +64,6 @@ config FB_DDC
>> tristate
>> depends on FB
>> select I2C_ALGOBIT
>> - select I2C
>> default n
>>
>> config FB_BOOT_VESA_SUPPORT
>> @@ -390,6 +389,7 @@ config FB_CYBER2000
>> config FB_CYBER2000_DDC
>> bool "DDC for CyberPro support"
>> depends on FB_CYBER2000
>> + depends on I2C=y || I2C=FB_CYBER2000
>> select FB_DDC
>> default y
>> help
>> @@ -634,7 +634,7 @@ config FB_BF537_LQ035
>> config FB_BFIN_7393
>> tristate "Blackfin ADV7393 Video encoder"
>> depends on FB && BLACKFIN
>> - select I2C
>> + depends on I2C
>> select FB_CFB_FILLRECT
>> select FB_CFB_COPYAREA
>> select FB_CFB_IMAGEBLIT
>> @@ -1026,6 +1026,7 @@ config FB_NVIDIA
>> config FB_NVIDIA_I2C
>> bool "Enable DDC Support"
>> depends on FB_NVIDIA
>> + depends on I2C=y || I2C=FB_NVIDIA
>> select FB_DDC
>> help
>> This enables I2C support for nVidia Chipsets. This is used
>> @@ -1073,6 +1074,7 @@ config FB_RIVA
>> config FB_RIVA_I2C
>> bool "Enable DDC Support"
>> depends on FB_RIVA
>> + depends on I2C=y || I2C=FB_RIVA
>> select FB_DDC
>> help
>> This enables I2C support for nVidia Chipsets. This is used
>> @@ -1102,6 +1104,7 @@ config FB_RIVA_BACKLIGHT
>> config FB_I740
>> tristate "Intel740 support"
>> depends on FB && PCI
>> + depends on I2C
>> select FB_MODE_HELPERS
>> select FB_CFB_FILLRECT
>> select FB_CFB_COPYAREA
>> @@ -1155,6 +1158,7 @@ config FB_I810_GTF
>> config FB_I810_I2C
>> bool "Enable DDC Support"
>> depends on FB_I810 && FB_I810_GTF
>> + depends on I2C=y || I2C=FB_I810
>> select FB_DDC
>> help
>>
>> @@ -1206,6 +1210,7 @@ config FB_INTEL_DEBUG
>> config FB_INTEL_I2C
>> bool "DDC/I2C for Intel framebuffer support"
>> depends on FB_INTEL
>> + depends on I2C=y || I2C=FB_INTEL
>> select FB_DDC
>> default y
>> help
>> @@ -1285,6 +1290,7 @@ config FB_MATROX_G
>> config FB_MATROX_I2C
>> tristate "Matrox I2C support"
>> depends on FB_MATROX
>> + depends on I2C
>> select FB_DDC
>> ---help---
>> This drivers creates I2C buses which are needed for accessing the
>> @@ -1350,6 +1356,7 @@ config FB_RADEON
>> config FB_RADEON_I2C
>> bool "DDC/I2C for ATI Radeon support"
>> depends on FB_RADEON
>> + depends on I2C=y || I2C=FB_RADEON
>> select FB_DDC
>> default y
>> help
>> @@ -1460,6 +1467,7 @@ config FB_S3
>> config FB_S3_DDC
>> bool "DDC for S3 support"
>> depends on FB_S3
>> + depends on I2C=y || I2C=FB_S3
>> select FB_DDC
>> default y
>> help
>> @@ -1485,6 +1493,7 @@ config FB_SAVAGE
>> config FB_SAVAGE_I2C
>> bool "Enable DDC2 Support"
>> depends on FB_SAVAGE
>> + depends on I2C=y || I2C=FB_SAVAGE
>> select FB_DDC
>> help
>> This enables I2C support for S3 Savage Chipsets. This is used
>> @@ -1629,6 +1638,7 @@ config FB_3DFX_ACCEL
>> config FB_3DFX_I2C
>> bool "Enable DDC/I2C support"
>> depends on FB_3DFX
>> + depends on I2C=y || I2C=FB_3DFX
>> select FB_DDC
>> default y
>> help
>> @@ -1669,6 +1679,7 @@ config FB_VT8623
>> config FB_TRIDENT
>> tristate "Trident/CyberXXX/CyberBlade support"
>> depends on FB && PCI
>> + depends on I2C
>> select FB_CFB_FILLRECT
>> select FB_CFB_COPYAREA
>> select FB_CFB_IMAGEBLIT
>> @@ -2299,8 +2310,8 @@ endchoice
>>
>> config FB_MB862XX_I2C
>> bool "Support I2C bus on MB862XX GDC"
>> - depends on FB_MB862XX && I2C
>> - depends on FB_MB862XX=m || I2C=y
>> + depends on FB_MB862XX
>> + depends on I2C=y || I2C=FB_MB862XX
>> default y
>> help
>> Selecting this option adds Coral-P(A)/Lime GDC I2C bus adapter


--
~Randy