Re: [PATCH v2 3/4] drm/bridge: dw-hdmi: add cec driver

[Neil Armstrong <narmstrong@baylibre.com>]

On 08/02/2017 03:14 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wednesday 02 Aug 2017 08:47:23 Hans Verkuil wrote:
>> On 08/02/2017 12:32 AM, Laurent Pinchart wrote:
>>>> +
>>>> +	cec_register_cec_notifier(cec->adap, cec->notify);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int dw_hdmi_cec_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct dw_hdmi_cec *cec = platform_get_drvdata(pdev);
>>>> +
>>>> +	cec_unregister_adapter(cec->adap);
>>>> +	cec_notifier_put(cec->notify);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static struct platform_driver dw_hdmi_cec_driver = {
>>>> +	.probe	= dw_hdmi_cec_probe,
>>>> +	.remove	= dw_hdmi_cec_remove,
>>>> +	.driver = {
>>>> +		.name = "dw-hdmi-cec",
>>>> +	},
>>>> +};
>>>> +module_platform_driver(dw_hdmi_cec_driver);
>>>
>>> Is there a particular reason why this has to be a separate module instead
>>> of simply calling the CEC init/cleanup functions directly from the main
>>> dw-hdmi driver ?
>>
>> Not all SoCs that use dw-hdmi also use the dw-hdmi CEC implementation. Some
>> use their own implementation (amlogic).
> 
> Lovely. Of course we need to reinvent the wheel every time, where would the 
> fun be otherwise ?
> 
>> So by implementing the cec-notifier in the dw-hdmi driver and keeping dw-
>> hdmi CEC separate you can easily choose whether or not you want to use this
>> CEC driver or another SoC CEC driver.
> 
> I'm certainly fine with such a split, but I don't think it requires a separate 
> platform_driver. We could use a similar approach as with the HDMI PHY that can 
> also differ between SoCs. The PHY is identified at runtime when possible, and 
> the SoC-specific glue code can override that with a few data fields and 
> function pointers.

The HDMI CEC is in the Synopsys IP like the I2S or AHB Audio, and is optional.
So it's legitimate to handle it in a similar fashion.
Amlogic has a separate custom CEC implementation, having an optional driver
makes it possible to avoid loading unnecessary code like the AHB audio driver
for non-freescale platforms.

> 
>>>> +MODULE_AUTHOR("Russell King <rmk+kernel@armlinux.org.uk>");
>>>> +MODULE_DESCRIPTION("Synopsys Designware HDMI CEC driver for i.MX");
>>>> +MODULE_LICENSE("GPL");
>>>> +MODULE_ALIAS(PLATFORM_MODULE_PREFIX "dw-hdmi-cec");
> 

Yes let's get this merged !

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