Re: [PATCH v2 6/7] drm/i2c: tda998x: add CEC support

From: Hans Verkuil <hverkuil@xs4all.nl>
To: Russell King - ARM Linux <linux@armlinux.org.uk>
Cc: David Airlie <airlied@linux.ie>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 6/7] drm/i2c: tda998x: add CEC support
Date: Fri, 8 Dec 2017 13:14:15 +0100	[thread overview]
Message-ID: <7ddc918f-4fd9-6e8c-a76c-964ad8f6a928@xs4all.nl> (raw)
In-Reply-To: <20171208115714.GA10595@n2100.armlinux.org.uk>

On 12/08/2017 12:57 PM, Russell King - ARM Linux wrote:
> On Wed, Dec 06, 2017 at 02:50:44PM +0100, Hans Verkuil wrote:
>> Hi Russell,
>>
>> Thanks for this patch series!
>>
>> On 12/06/17 13:35, Russell King wrote:
>>> The TDA998x is a HDMI transmitter with a TDA9950 CEC engine integrated
>>> onto the same die.  Add support for the TDA9950 CEC engine to the
>>> TDA998x driver.
>>>
>>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>>> ---
>>>  drivers/gpu/drm/i2c/Kconfig       |   1 +
>>>  drivers/gpu/drm/i2c/tda998x_drv.c | 209 +++++++++++++++++++++++++++++++++++---
>>>  2 files changed, 196 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
>>> index 3a232f5ff0a1..096d2139e733 100644
>>> --- a/drivers/gpu/drm/i2c/Kconfig
>>> +++ b/drivers/gpu/drm/i2c/Kconfig
>>> @@ -22,6 +22,7 @@ config DRM_I2C_SIL164
>>>  config DRM_I2C_NXP_TDA998X
>>>  	tristate "NXP Semiconductors TDA998X HDMI encoder"
>>>  	default m if DRM_TILCDC
>>> +	select CEC_NOTIFIER
>>
>> I believe this should be 'select CEC_CORE if CEC_NOTIFIER', conform the
>> other drivers that do something similar.
>>
>> Otherwise if tda9950 is configured as a module, and this as built-in, then
>> cec is built as a module as well and this can't find the cec functions from
>> the module.
> 
> You mean when we have:
> 
> CONFIG_DRM_I2C_NXP_TDA998X=y
> CONFIG_DRM_I2C_NXP_TDA9950=m
> 
> ?
> 
> That appears to work fine with:
> 
> CONFIG_CEC_CORE=m
> CONFIG_CEC_NOTIFIER=y
> 
> in 4.14, as that's exactly the configuration I test with on Dove.  Maybe
> that's changed recently, or maybe I haven't noticed it not working (I
> can't test it at the moment, sorry.)
> 

I don't think that can work. In cec-notifier.h:

#if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)

And CEC_CORE is not reachable from the tda998x, so all the cec_notifier
functions become dummies.

It compiles, but the physical address never gets set.

'select CEC_CORE if CEC_NOTIFIER' will ensure CEC_CORE=y and the #if above
resolves to true.

Regards,

	Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel