On 17/07/17 14:05, Russell King - ARM Linux wrote: > On Mon, Jul 17, 2017 at 01:39:48PM +0200, Hans Verkuil wrote: >> On 17/07/17 11:05, Russell King - ARM Linux wrote: >>> On Mon, Jul 17, 2017 at 10:56:47AM +0200, Hans Verkuil wrote: >>>> Hi Russell, >>>> >>>> On 09/06/17 16:10, Russell King - ARM Linux wrote: >>>>> On Fri, Jun 09, 2017 at 03:56:39PM +0200, Neil Armstrong wrote: >>>>>> Yes, but on the Amlogic Meson plarform, the DW-HDMI CEC controller is >>>>>> not used, but a custom one, so this notifier is actually useful for >>>>>> this platform and maybe others. >>>>> >>>>> Is the CEC controller configured into dw-hdmi (is the config bit set?) >>>>> I'm just wondering if we're going to end up with two CEC drivers trying >>>>> to bind to the same notifier. >>>>> >>>>>> Should we really wait until I push the Amlogic AO CEC driver ? Having a >>>>>> notifier in the DW-HDMI driver won't harm anybody since it *will be used*. >>>>> >>>>> It sounds like this adds additional information that has been missing >>>>> from the review of my patches - and I suspect changes Hans' comments. >>>>> So, I'll wait, it seems pointless to try and update the patches when >>>>> it's not clear how to proceed due to other dependencies, especially >>>>> when it means that their existing state is what's required (I'm pleased >>>>> that I've held off modifying the patches so far.) >>>>> >>>>> If that means having to wait another kernel revision, then I guess that's >>>>> what will have to happen. >>>> >>>> Can you respin your patch series, keeping the notifier support? The CEC >>>> kernel config handling has been cleaned up (just select CEC_CORE and >>>> CEC_NOTIFIER) so you should be good to go. >>> >>> Not yet - the change to the way you're dealing with Kconfig in CEC is >>> fundamentally broken, and needs fixing before we can merge dw-hdmi-cec >>> support. >>> >>> As a result of these Kconfig changes, dw-hdmi-cec now fails if: >>> >>> 1. You build the CEC part as a module >>> 2. You build the HDMI part into the kernel >>> >>> This results in CEC_NOTIFIER=y and CEC_CORE=m, which, when the HDMI part >>> gets built, results in the stubs in the notifier code being used, rather >>> than the real functions. This in turn causes the CEC part to never >>> receive a physical address, which is therefore non-functional. >>> >>> I did have a patch to fix this, but it was never committed, and I got >>> busy with other stuff (so it ended up being git reset --hard away.) >>> >> >> This is more a DRM_DW_HDMI issue than a CEC issue IMHO. >> >> This will fix this: >> >> config DRM_DW_HDMI >> tristate >> select DRM_KMS_HELPER >> select REGMAP_MMIO >> select CEC_CORE if CEC_NOTIFIER <<<<<< >> >> config DRM_DW_HDMI_CEC >> tristate "Synopsis Designware CEC interface" >> depends on DRM_DW_HDMI >> select CEC_CORE >> select CEC_NOTIFIER >> help >> Support the CE interface which is part of the Synopsis >> Designware HDMI block. >> >> This makes sense: if DRM_DW_HDMI_CEC is disabled but another CEC module is >> used instead (as is apparently the case for amlogic), then the >> >> select CEC_CORE if CEC_NOTIFIER >> >> line ensures that CONFIG_CEC_CORE has the right m/y value. > > I disagree with this approach. > > If DRM_DW_HDMI=y and DRM_DW_HDMI_CEC=n, but some other driver is enabled > that selects CEC_NOTIFIER, then we end up with CEC_CORE forced enabled > through dw-hdmi, even though we haven't asked for the CEC part to be > enabled. If CEC_NOTIFIER is enabled by a CEC driver, then CEC_CORE will also be enabled (without CEC_CORE that driver wouldn't compile, obviously). So I don't see the problem. All the select...if does is make sure that the CEC_CORE can be reached from the HDMI driver if someone enabled the CEC notifier (and thus CEC_CORE). > You might as well have CEC_NOTIFIER itself select CEC_CORE, and be done > with it, because that's basically what this boils down to. That makes no sense. If CEC_NOTIFIER is set, then both the CEC driver and the HDMI driver have to select CEC_CORE to ensure the right dependency. If CEC_NOTIFIER is not set, then only the CEC driver has to select CEC_CORE. In that case the CEC code is typically either integrated into the HDMI driver or it is a standalone device like the USB pulse8-cec driver. Regards, Hans _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
