[PATCH 2/4] drm/bridge: dw-hdmi: add cec notifier support

[Hans Verkuil]

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