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

[Arnd Bergmann]

On Fri, Feb 2, 2018 at 1:21 AM, Randy Dunlap <rdunlap@xxxxxxxxxxxxx> wrote:
> 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. :)

Right, that should also be fixed, so DRM no longer includes I2C ;-)

At the moment, DRM is the most common cause for circular dependencies
because it has a number of 'select' statements for symbols that otherwise
are used with 'depends on'. We should probably address the 'select I2C'
portion in there, but also some of the others like:

drivers/gpu/drm/Kconfig:        select POWER_SUPPLY
drivers/gpu/drm/Kconfig:        select HWMON
drivers/gpu/drm/Kconfig:        select FB
drivers/gpu/drm/udl/Kconfig: select USB
drivers/gpu/drm/sti/Kconfig: select OF
drivers/gpu/drm/sti/Kconfig: select RESET_CONTROLLER
drivers/gpu/drm/etnaviv/Kconfig:        select CMA if HAVE_DMA_CONTIGUOUS
drivers/gpu/drm/etnaviv/Kconfig:        select TMPFS
drivers/gpu/drm/i915/Kconfig.debug:        select DEBUG_FS
drivers/gpu/drm/i915/Kconfig.debug:        select PREEMPT_COUNT
drivers/gpu/drm/i915/Kconfig.debug:     select TRACING
drivers/gpu/drm/i915/Kconfig.debug:     select FAULT_INJECTION
drivers/gpu/drm/mediatek/Kconfig:       select MEMORY
drivers/gpu/drm/mediatek/Kconfig:       select GENERIC_PHY
drivers/gpu/drm/msm/Kconfig:    select REGULATOR

> 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

I don't think that would address the issue of 'defconfig' builds losing
I2C support when it's no longer automatically selection. On x86, this
is not an issue, as X86 always enables I2C. For the rest, we could
do a hack like this:

--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -8,6 +8,7 @@ config I2C
        tristate "I2C support"
        select RT_MUTEXES
        select IRQ_DOMAIN
+       default DRM || FB
        ---help---
          I2C (pronounce: I-squared-C) is a slow serial bus protocol used in
          many micro controller applications and developed by Philips.  SMBus,

which would let all 'defconfig' versions keep working. It's a bit ugly, but
at least wouldn't cause other circular dependencies.

     Arnd