Re: [PATCH 0/4] musb fixes for v4.9-rc cycle
On 10/11/16 23:25, Laurent Pinchart wrote: [2.766174] musb_bus_suspend 2586: trying to suspend as a_idle while active printed in a loop at boot time. I've traced musb->is_active being set to 1 in musb_start() with > Actually disabling CONFIG_USB_MUSB_HDRC gets rid of the problem, quite > understandably. It's not a fix though, just a workaround. Are you interested > in investigating this ? I'm seeing this too. Did you manage to boot your Panda? If I disable CONFIG_USB_MUSB_HDRC, I don't see that error print, but the ethernet doesn't work, and so my nfsroot doesn't mount. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH 0/4] musb fixes for v4.9-rc cycle
On 23/11/16 17:49, Laurent Pinchart wrote: > Hi Tomi, > > On Wednesday 23 Nov 2016 12:14:17 Tomi Valkeinen wrote: >> On 10/11/16 23:25, Laurent Pinchart wrote: >>>>>> [2.766174] musb_bus_suspend 2586: trying to suspend as a_idle while >>>>>> active >>>>>> >>>>>> printed in a loop at boot time. I've traced musb->is_active being set >>>>>> to 1 in musb_start() with >>> >>> Actually disabling CONFIG_USB_MUSB_HDRC gets rid of the problem, quite >>> understandably. It's not a fix though, just a workaround. Are you >>> interested in investigating this ? >> >> I'm seeing this too. Did you manage to boot your Panda? If I disable >> CONFIG_USB_MUSB_HDRC, I don't see that error print, but the ethernet >> doesn't work, and so my nfsroot doesn't mount. > > Do you use ethernet over USB gadget or over LAN9514 (smsx95xx) ? The LAN9514 > is connected to the EHCI USB interface so it shouldn't depend on MUSB. smsx95xx. I didn't look at it further, so possibly there's something else broken/changed that prevented the ethernet from working. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH 0/4] musb fixes for v4.9-rc cycle
On 23/11/16 18:34, Tony Lindgren wrote: > OK. And what changes to your current .config make the musb_bus_suspend() > issues show up? > > If it happens with USB-B cable from host to musb with musb set to host > only mode I'm not suprised there are errors :) I have no USB cables connected. I have attached my config. If CONFIG_USB_MUSB_HDRC is disabled, the musb error goes away. Tomi # # Automatically generated file; DO NOT EDIT. # Linux/arm 4.9.0-rc6 Kernel Configuration # CONFIG_ARM=y CONFIG_ARM_HAS_SG_CHAIN=y CONFIG_NEED_SG_DMA_LENGTH=y CONFIG_ARM_DMA_USE_IOMMU=y CONFIG_ARM_DMA_IOMMU_ALIGNMENT=8 CONFIG_MIGHT_HAVE_PCI=y CONFIG_SYS_SUPPORTS_APM_EMULATION=y CONFIG_HAVE_PROC_CPU=y CONFIG_STACKTRACE_SUPPORT=y CONFIG_LOCKDEP_SUPPORT=y CONFIG_TRACE_IRQFLAGS_SUPPORT=y CONFIG_RWSEM_XCHGADD_ALGORITHM=y CONFIG_ARCH_HAS_BANDGAP=y CONFIG_FIX_EARLYCON_MEM=y CONFIG_GENERIC_HWEIGHT=y CONFIG_GENERIC_CALIBRATE_DELAY=y CONFIG_NEED_DMA_MAP_STATE=y CONFIG_ARCH_SUPPORTS_UPROBES=y CONFIG_VECTORS_BASE=0x CONFIG_ARM_PATCH_PHYS_VIRT=y CONFIG_GENERIC_BUG=y CONFIG_PGTABLE_LEVELS=2 CONFIG_DEFCONFIG_LIST="/lib/modules/$UNAME_RELEASE/.config" CONFIG_IRQ_WORK=y CONFIG_BUILDTIME_EXTABLE_SORT=y # # General setup # CONFIG_INIT_ENV_ARG_LIMIT=32 CONFIG_CROSS_COMPILE="" # CONFIG_COMPILE_TEST is not set CONFIG_LOCALVERSION="" CONFIG_LOCALVERSION_AUTO=y CONFIG_HAVE_KERNEL_GZIP=y CONFIG_HAVE_KERNEL_LZMA=y CONFIG_HAVE_KERNEL_XZ=y CONFIG_HAVE_KERNEL_LZO=y CONFIG_HAVE_KERNEL_LZ4=y # CONFIG_KERNEL_GZIP is not set CONFIG_KERNEL_LZMA=y # CONFIG_KERNEL_XZ is not set # CONFIG_KERNEL_LZO is not set # CONFIG_KERNEL_LZ4 is not set CONFIG_DEFAULT_HOSTNAME="(none)" CONFIG_SWAP=y CONFIG_SYSVIPC=y CONFIG_SYSVIPC_SYSCTL=y CONFIG_POSIX_MQUEUE=y CONFIG_POSIX_MQUEUE_SYSCTL=y CONFIG_CROSS_MEMORY_ATTACH=y CONFIG_FHANDLE=y # CONFIG_USELIB is not set CONFIG_AUDIT=y CONFIG_HAVE_ARCH_AUDITSYSCALL=y CONFIG_AUDITSYSCALL=y CONFIG_AUDIT_WATCH=y CONFIG_AUDIT_TREE=y # # IRQ subsystem # CONFIG_GENERIC_IRQ_PROBE=y CONFIG_GENERIC_IRQ_SHOW=y CONFIG_GENERIC_IRQ_SHOW_LEVEL=y CONFIG_HARDIRQS_SW_RESEND=y CONFIG_GENERIC_IRQ_CHIP=y CONFIG_IRQ_DOMAIN=y CONFIG_IRQ_DOMAIN_HIERARCHY=y CONFIG_HANDLE_DOMAIN_IRQ=y # CONFIG_IRQ_DOMAIN_DEBUG is not set CONFIG_IRQ_FORCED_THREADING=y CONFIG_SPARSE_IRQ=y CONFIG_ARCH_CLOCKSOURCE_DATA=y CONFIG_GENERIC_TIME_VSYSCALL=y CONFIG_GENERIC_CLOCKEVENTS=y CONFIG_ARCH_HAS_TICK_BROADCAST=y CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y # # Timers subsystem # CONFIG_TICK_ONESHOT=y CONFIG_NO_HZ_COMMON=y # CONFIG_HZ_PERIODIC is not set CONFIG_NO_HZ_IDLE=y # CONFIG_NO_HZ_FULL is not set CONFIG_NO_HZ=y CONFIG_HIGH_RES_TIMERS=y # # CPU/Task time and stats accounting # CONFIG_TICK_CPU_ACCOUNTING=y # CONFIG_VIRT_CPU_ACCOUNTING_GEN is not set # CONFIG_IRQ_TIME_ACCOUNTING is not set CONFIG_BSD_PROCESS_ACCT=y # CONFIG_BSD_PROCESS_ACCT_V3 is not set # CONFIG_TASKSTATS is not set # # RCU Subsystem # CONFIG_PREEMPT_RCU=y # CONFIG_RCU_EXPERT is not set CONFIG_SRCU=y # CONFIG_TASKS_RCU is not set CONFIG_RCU_STALL_COMMON=y # CONFIG_TREE_RCU_TRACE is not set # CONFIG_RCU_EXPEDITE_BOOT is not set CONFIG_BUILD_BIN2C=y CONFIG_IKCONFIG=y CONFIG_IKCONFIG_PROC=y CONFIG_LOG_BUF_SHIFT=16 CONFIG_LOG_CPU_MAX_BUF_SHIFT=12 CONFIG_NMI_LOG_BUF_SHIFT=13 CONFIG_GENERIC_SCHED_CLOCK=y CONFIG_CGROUPS=y CONFIG_PAGE_COUNTER=y CONFIG_MEMCG=y CONFIG_MEMCG_SWAP=y CONFIG_MEMCG_SWAP_ENABLED=y CONFIG_BLK_CGROUP=y # CONFIG_DEBUG_BLK_CGROUP is not set CONFIG_CGROUP_WRITEBACK=y CONFIG_CGROUP_SCHED=y CONFIG_FAIR_GROUP_SCHED=y CONFIG_CFS_BANDWIDTH=y CONFIG_RT_GROUP_SCHED=y # CONFIG_CGROUP_PIDS is not set CONFIG_CGROUP_FREEZER=y CONFIG_CPUSETS=y CONFIG_PROC_PID_CPUSET=y CONFIG_CGROUP_DEVICE=y CONFIG_CGROUP_CPUACCT=y CONFIG_CGROUP_PERF=y # CONFIG_CGROUP_DEBUG is not set # CONFIG_CHECKPOINT_RESTORE is not set CONFIG_NAMESPACES=y CONFIG_UTS_NS=y CONFIG_IPC_NS=y # CONFIG_USER_NS is not set CONFIG_PID_NS=y CONFIG_NET_NS=y # CONFIG_SCHED_AUTOGROUP is not set # CONFIG_SYSFS_DEPRECATED is not set # CONFIG_RELAY is not set CONFIG_BLK_DEV_INITRD=y CONFIG_INITRAMFS_SOURCE="" CONFIG_RD_GZIP=y CONFIG_RD_BZIP2=y CONFIG_RD_LZMA=y CONFIG_RD_XZ=y CONFIG_RD_LZO=y CONFIG_RD_LZ4=y CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=y # CONFIG_CC_OPTIMIZE_FOR_SIZE is not set CONFIG_SYSCTL=y CONFIG_ANON_INODES=y CONFIG_HAVE_UID16=y CONFIG_BPF=y CONFIG_EXPERT=y CONFIG_UID16=y CONFIG_MULTIUSER=y # CONFIG_SGETMASK_SYSCALL is not set CONFIG_SYSFS_SYSCALL=y # CONFIG_SYSCTL_SYSCALL is not set CONFIG_KALLSYMS=y CONFIG_KALLSYMS_ALL=y # CONFIG_KALLSYMS_ABSOLUTE_PERCPU is not set CONFIG_KALLSYMS_BASE_RELATIVE=y CONFIG_PRINTK=y CONFIG_PRINTK_NMI=y CONFIG_BUG=y CONFIG_ELF_CORE=y CONFIG_BASE_FULL=y CONFIG_FUTEX=y CONFIG_EPOLL=y CONFIG_SIGNALFD=y CONFIG_TIMERFD=y CONFIG_EVENTFD=y # CONFIG_BPF_SYSCALL is not set CONFIG_SHMEM=y CONFIG_AIO=y CONFIG_ADVISE_SYSCALLS=y # CONFIG_USERFAULTFD is not set CONFIG_MEMBARRIER=y # CONFIG_EMBEDDED is not set CONFIG_HAVE_PERF_EVENTS=y CONFIG_PERF_USE_VMALLOC=y # # Kernel Performance Events And Counters # CONFIG_PERF_EVE
Re: [RFC PATCH 1/4] arm: omap: Add phy binding info for musb in plat data
Hi, On 28/05/13 08:18, Kishon Vijay Abraham I wrote: > Hi Tony, > > On Friday 17 May 2013 06:52 PM, Kishon Vijay Abraham I wrote: >> In order for controllers to get PHY in case of non dt boot, the phy >> binding information (phy label) should be added in the platform >> data of the controller. > > This series would be needed to get MUSB working in OMAP3 boards for > non-dt boot case. Do you think this is good enough to go in this rc cycle? Did this or some other solution go forward? I'm still unable to boot with usb-gadget-ethernet with v3.10-rc5. Tomi signature.asc Description: OpenPGP digital signature
Re: [RFC PATCH 1/4] arm: omap: Add phy binding info for musb in plat data
On 14/06/13 08:47, Tony Lindgren wrote: > * Kishon Vijay Abraham I [130613 22:41]: >> Hi, >> >> On Thursday 13 June 2013 06:35 PM, Tomi Valkeinen wrote: >>> Hi, >>> >>> On 28/05/13 08:18, Kishon Vijay Abraham I wrote: >>>> Hi Tony, >>>> >>>> On Friday 17 May 2013 06:52 PM, Kishon Vijay Abraham I wrote: >>>>> In order for controllers to get PHY in case of non dt boot, the phy >>>>> binding information (phy label) should be added in the platform >>>>> data of the controller. >>>> >>>> This series would be needed to get MUSB working in OMAP3 boards for >>>> non-dt boot case. Do you think this is good enough to go in this rc cycle? >>> >>> Did this or some other solution go forward? I'm still unable to boot >>> with usb-gadget-ethernet with v3.10-rc5. >> >> No. I think Tony is ok to take this only during next merge window. > > Yes I'll apply them to omap-for-v3.11/fixes-non-critical. We really > should have basic functionaly tested and working always before the > merge window so we only need to do minimal fixes during the -rc cycle. I'm mostly interested in the USB gadget ethernet for the boards I have, but if I'm not mistaken, all USB gadget support for many OMAP boards is broken in v3.10. Shouldn't that be fixed, no matter if it's a minimal fix or not? Or is there some other, more minimal, way to fix this? Tomi signature.asc Description: OpenPGP digital signature
Re: [RFC PATCH 1/4] arm: omap: Add phy binding info for musb in plat data
On 14/06/13 10:33, Tony Lindgren wrote: > If we want to fix something this late in the merge window, the patches > must have a clear description what caused the regression and what happens > without the patches. These patches don't have that. And they are marked > RFC also. So actually I'm not applying any of them before the regression > descriptions are there and the patches have been reposted without RFC > and have sufficient acks from people. No disagreement there. Kishon, I tested the patches on top of v3.10-rc5, booting with nfs root via usb gadget eth: Overo: works Beagle: works, but I need to reconnect the usb cable after kernel is up Beagle-xm: doesn't work. The cable is detected correctly, though Panda: works The problems with Beagles are there even without these patches, so they do make things better (fix Overo). Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH] build some drivers only when compile-testing
On 18/06/13 07:51, Michal Marek wrote: >> Sam Ravnborg (the kconfig ex-maintainer) once wrote that he doesn't want >> to extend the kconfig language for this purpose (which I support). That >> a config option is fine and sufficient in this case [1]. Except he >> called the config option "SHOW_ALL_DRIVERS". Adding the current >> maintainer to CCs ;). > > I agree with Sam. 'depends on XY || COMPILE_TEST' is quite > self-explanatory. And even if it's not, you can look up the help text > for COMPILE_TEST. With "archdepends on" or "available on", you need to > know what to look for to override the dependency. I would rather have "depends on", when the code actually depends on something (i.e. you can't compile/load the code otherwise), and "used on"/"available on" when the code is just normally not used except on the listed platforms (but nothing prevents from compiling and using the code on all platforms). But I'm fine with COMPILE_TEST or similar, I guess it's an acceptable compromise and trivial to implement. Even if we had "used on" we'd still need to update the Kconfig file when the code is being used on a new platform, just like with COMPILE_TEST. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH] build some drivers only when compile-testing
On 17/06/13 23:05, Jiri Slaby wrote: > The last point I inclined to the Greg's argument to remove the EXPERT > dependency. > > So currently I have what is attached... Comments? The patch looks a bit odd with the USB_CHIPIDEA_IMX parts. You're not adding COMPILE_TEST there, but you're adding a totally new config option, and also changing the Makefile. Maybe that's ok, but there's no mention about this in the desc. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH] build some drivers only when compile-testing
On 19/06/13 10:12, Jiri Slaby wrote: > On 06/19/2013 09:10 AM, Tomi Valkeinen wrote: >> On 17/06/13 23:05, Jiri Slaby wrote: >> >>> The last point I inclined to the Greg's argument to remove the >>> EXPERT dependency. >>> >>> So currently I have what is attached... Comments? >> >> The patch looks a bit odd with the USB_CHIPIDEA_IMX parts. You're >> not adding COMPILE_TEST there, but you're adding a totally new >> config option, and also changing the Makefile. > > Look: > > +config USB_CHIPIDEA_IMX > + bool "ChipIdea IMX support" > + depends on OF_DEVICE && (ARM || COMPILE_TEST) > > COMPILE_TEST added here My point was that you're not adding COMPILE_TEST to an existing config option, you're creating a totally new config option. As I said, that's probably ok, but it'd be nice to mention extra things like that in the desc. The pedantic approach would be to do the makefile and Kconfig change in an earlier patch, and then add only COMPILE_TEST. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH] build some drivers only when compile-testing
Hi, On 22/05/13 12:18, Jiri Slaby wrote: > Some drivers can be built on more platforms than they run on. This > causes users and distributors packaging burden when they have to > manually deselect some drivers from their allmodconfigs. Or sometimes > it is even impossible to disable the drivers without patching the > kernel. > > Introduce a new config option COMPILE_TEST and make all those drivers > to depend on the platform they run on, or on the COMPILE_TEST option. > Now, when users/distributors choose COMPILE_TEST=n they will not have > the drivers in their allmodconfig setups, but developers still can > compile-test them with COMPILE_TEST=y. > > Now the drivers where we use this new option: > * PTP_1588_CLOCK_PCH: The PCH EG20T is only compatible with Intel Atom > processors so it should depend on x86. > * FB_GEODE: Geode is 32-bit only so only enable it for X86_32. > * USB_CHIPIDEA_IMX: The OF_DEVICE dependency will be met on powerpc > systems -- which do not actually support the hardware via that > method. I had this exact same idea some time ago. The mail below contains some of my reasoning for this: http://comments.gmane.org/gmane.linux.kbuild.devel/9829 I proposed a new Kconfig keyword, but Sam was quite against it as the Kconfig language already does what is required. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 0/3] ARM: OMAP2+: USB Host bug fixes for 3.13 rc
On 2013-12-03 16:25, Roger Quadros wrote: > Hi, > > This is a follow up solution to the original series in [1] > > The first patch fixes the OMAP4 Panda USB detection problems on 3.13-rc1 > with u-boot v2013.10. > > The remaining 2 patches are required if SOFTRESET needs to be done for the > USB Host module on OMAP3 platforms. > > Patch 2 fixes the hwmod RESET logic to prevent multiple SOFTRESETs > being issued to the modules. This multiple SOFTRESET was causing problems > with OMAP3 USB Host module. On Beagleboard C4 this is seen as failure to > mount NFS root over external USB to Ethernet device. > > This might be the reason why HWMOD_INIT_NO_RESET was used for the OMAP > USB host module in OMAP3 hwmod data and just carried forward in OMAP4 > and OMAP5 hwmod data as well. > > cheers, > -roger Tested on Panda and Beagle xM. Works fine for me. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO
On 18/10/14 00:13, Jani Nikula wrote: > Documentation/kbuild/kconfig-language.txt warns to use select with care, > and in general use select only for non-visible symbols and for symbols > with no dependencies, because select will force a symbol to a value > without visiting the dependencies. > > Select has become particularly problematic, interdependently, with the > BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO configs. For example: > > scripts/kconfig/conf --randconfig Kconfig > KCONFIG_SEED=0x48312B00 > warning: (DRM_RADEON && DRM_NOUVEAU && DRM_I915 && DRM_GMA500 && > DRM_SHMOBILE && DRM_TILCDC && FB_BACKLIGHT && FB_MX3 && USB_APPLEDISPLAY > && FB_OLPC_DCON && ASUS_LAPTOP && SONY_LAPTOP && THINKPAD_ACPI && > EEEPC_LAPTOP && ACPI_CMPC && SAMSUNG_Q10) selects BACKLIGHT_CLASS_DEVICE > which has unmet direct dependencies (HAS_IOMEM && BACKLIGHT_LCD_SUPPORT) > warning: (DRM_RADEON && DRM_NOUVEAU && DRM_I915 && DRM_GMA500 && > DRM_SHMOBILE && DRM_TILCDC && FB_BACKLIGHT && FB_MX3 && USB_APPLEDISPLAY > && FB_OLPC_DCON && ASUS_LAPTOP && SONY_LAPTOP && THINKPAD_ACPI && > EEEPC_LAPTOP && ACPI_CMPC && SAMSUNG_Q10) selects BACKLIGHT_CLASS_DEVICE > which has unmet direct dependencies (HAS_IOMEM && BACKLIGHT_LCD_SUPPORT) > > With tristates it's possible to end up selecting FOO=y depending on > BAR=m in the config, which gets discovered at build time, not config > time, like reported in the thread referenced below. > > Do the following to fix the dependencies: > > * Depend on instead of select BACKLIGHT_CLASS_DEVICE everywhere. Drop > select BACKLIGHT_LCD_SUPPORT in such cases, as it's a dependency of > BACKLIGHT_CLASS_DEVICE. > > * Remove config FB_BACKLIGHT altogether, and replace it with a > dependency on BACKLIGHT_CLASS_DEVICE. All configs selecting > FB_BACKLIGHT select or depend on FB anyway, so we can simplify. > > * Depend on (ACPI && ACPI_VIDEO) || ACPI=n in several places instead of > selecting ACPI_VIDEO and a number of its dependencies if ACPI is > enabled. This is tied to backlight, as ACPI_VIDEO depends on > BACKLIGHT_CLASS_DEVICE. > > * Replace a couple of select INPUT/VT with depends as it seemed to be > necessary. While doing 'depends on' instead of 'select' is an "easy" fix for this, I do dislike it quite a bit. It's a major pain to go around the kernel config, trying to find all the dependencies that a particular driver wants. If I need fb-foobar, I should just be able to enable it, instead of first searching and selecting its minor dependencies individually. So, not a NACK, but a "isn't there an another way to fix this?". Looking at backlight... BACKLIGHT_LCD_SUPPORT seems to be a "meta" option, it only enables a Kconfig submenu. So I think we could just remove the whole BACKLIGHT_LCD_SUPPORT option. But if we do that, all the items in drivers/video/backlight/Kconfig with default 'y' or 'm' would get enabled by default, so I think we should remove the 'default's from that file. That makes sense in any case, as I don't see why "HP Jornada 700 series LCD Driver" should be "default y". BACKLIGHT_CLASS_DEVICE doesn't depend on anything except BACKLIGHT_LCD_SUPPORT, so after removing BACKLIGHT_LCD_SUPPORT it should be safe to 'select' BACKLIGHT_CLASS_DEVICE. BACKLIGHT_CLASS_DEVICE could be made a hidden option, and the drivers in drivers/video/backlight/Kconfig which are under BACKLIGHT_CLASS_DEVICE could be made to select BACKLIGHT_CLASS_DEVICE instead. That doesn't exactly fix anything, but I think it makes sense as BACKLIGHT_CLASS_DEVICE is something that's selected from all around the kernel, so it should be a selectable "library" instead of a Kconfig menu option. I didn't look at the ACPI_VIDEO side, so no idea how messy that is. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO
On 23/10/14 11:10, Daniel Vetter wrote: > If we want to make BACKLIGHT_CLASS_DEVICE into a library thing then I > guess we could do that, but we must then also drag it out of all the other > meta options to make sure it's always available. No need I think to ditch BACKLIGHT_CLASS_DEVICE only depends on HAS_IOMEM and BACKLIGHT_LCD_SUPPORT so there are no other meta options to avoid. HAS_IOMEM comes from drivers/video/Kconfig's "Graphics support", and I guess we can ignore it. > the entire BACKLIGHT_LCD_SUPPORT meta option. And then everyone could > select it. I don't quite understand what purpose does BACKLIGHT_LCD_SUPPORT serve. It doesn't enable any code, it just opens up new Kconfig options. Why can't the Kconfig options be always available? It's just another option to 'select', without any reason I can see. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO
On 27/10/14 13:59, Jani Nikula wrote: >> While doing 'depends on' instead of 'select' is an "easy" fix for this, >> I do dislike it quite a bit. It's a major pain to go around the kernel >> config, trying to find all the dependencies that a particular driver >> wants. If I need fb-foobar, I should just be able to enable it, instead >> of first searching and selecting its minor dependencies individually. > > Agreed, but I don't think that's specific to this patch. Well, no, the generic problem is not specific to this patch, but we can avoid the issue with proper use of 'select' (at least in some cases), which is specific to this patch. >> So, not a NACK, but a "isn't there an another way to fix this?". > > I think the real answer would be to fix kconfig to also show menu items > whose dependencies are not met, and then recursively enabling the > dependencies when the item is enabled. Beyond my scope. > >> Looking at backlight... BACKLIGHT_LCD_SUPPORT seems to be a "meta" >> option, it only enables a Kconfig submenu. >> >> So I think we could just remove the whole BACKLIGHT_LCD_SUPPORT option. >> But if we do that, all the items in drivers/video/backlight/Kconfig with >> default 'y' or 'm' would get enabled by default, so I think we should >> remove the 'default's from that file. That makes sense in any case, as I >> don't see why "HP Jornada 700 series LCD Driver" should be "default y". >> >> BACKLIGHT_CLASS_DEVICE doesn't depend on anything except >> BACKLIGHT_LCD_SUPPORT, so after removing BACKLIGHT_LCD_SUPPORT it should >> be safe to 'select' BACKLIGHT_CLASS_DEVICE. >> >> BACKLIGHT_CLASS_DEVICE could be made a hidden option, and the drivers in >> drivers/video/backlight/Kconfig which are under BACKLIGHT_CLASS_DEVICE >> could be made to select BACKLIGHT_CLASS_DEVICE instead. > > I think it should be possible to choose between y and m when it's If I'm not mistaken, if CONFIG_FOO is 'm', and it 'select's CONFIG_BAR, and CONFIG_BAR is tristate, then CONFIG_BAR will be set to 'm'. > selected, and it should be possible to enable it when it's not selected > by any drivers. I'm not sure a hidden option is good for that. Why would you want to enable it if no one uses it? Does BACKLIGHT_CLASS_DEVICE enable something even if no driver uses it? >> That doesn't exactly fix anything, but I think it makes sense as >> BACKLIGHT_CLASS_DEVICE is something that's selected from all around the >> kernel, so it should be a selectable "library" instead of a Kconfig menu >> option. > > At least for drm/i915 BACKLIGHT_CLASS_DEVICE is "an option". We use it > if it's enabled, but we are just fine if it's not. I've learned the way > to express that is > > depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n > > but I don't think there's a way to express that in terms of select, is > there? The dependency above guarantees there's no DRM_I915=y and > BACKLIGHT_CLASS_DEVICE=m combo which would fail. And this, btw, is where > this whole patch got started, as select didn't handle that properly. If backlight support is considered an option for drm/i915, then I think there should be a Kconfig option for i915 to enable backlight support, which in turn selects BACKLIGHT_CLASS_DEVICE. And that select will force BACKLIGHT_CLASS_DEVICE to be built-in if drm/i915 is built-in. Oh, but it doesn't work optimally with modules. The new option needed for that would be boolean, so BACKLIGHT_CLASS_DEVICE would always be either y or n. Sigh... >> I didn't look at the ACPI_VIDEO side, so no idea how messy that is. > > Basically it's another dependency on BACKLIGHT_CLASS_DEVICE. I can only > imagine trying to solve this problem with select is going to end up in > recursive dependencies that spread out and need changing about as wide > as this patch. If ACPI_VIDEO uses select to enable BACKLIGHT_CLASS_DEVICE, then, I think, selecting ACPI_VIDEO will also select BACKLIGHT_CLASS_DEVICE. So I don't right away see any recursive dependencies. Or what did you have in mind? > In the end, I agree with the problem you have with this patch, but yet I > think it's the right thing to do in terms of expressing the > dependencies. Well, dri/i915 doesn't exactly depend on backlight, if I understood you correctly. Instead, backlight is an option for dri/i915, and you kind of hack it to be implemented with that 'depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n'. I guess it's debatable whether drivers should automatically use features in the kernel if they happen to be enabled in the Kconfig, or should they be individually enabled for that driver. I personally like the latter option, as it allows more precise control, but it probably also depends on the feature in question. I also think the 'depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n' pattern is quite... interesting (i.e. sounds like a hack to me =). Tomi signature.asc Description: OpenPGP digital signature
