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

[hverkuil@xs4all.nl (Hans Verkuil)]

Hi Russell,

On 07/17/2017 02:23 PM, Hans Verkuil wrote:
> 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.

Just to make sure you aren't waiting for me to do anything: as far as I can tell
the Kconfig above will ensure the right dependencies. If you have an example
where this fails, then let me know. I am not planning on making any changes.
Frankly, I wouldn't know what to change since AFAICT it is all working with
the above Kconfig.

Regards,

	Hans