RE: [PATCH v7 0/4] drm: add support for Cadence MHDP DPI/DP bridge.

[Damian Kos <dkos@cadence.com>]

>Damian, ping.
>
>On 31/01/2019 14:08, Tomi Valkeinen wrote:
>> Hi,
>> 
>> On 30/01/2019 13:03, Damian Kos wrote:
>>> Hello!
>>>
>>> This is the series of patches that will add support for the Cadence's DPI/DP
>>> bridge. Please note that this is a preliminary version of the driver and there
>>> will be more patches in the future with updates, fixes and improvements.
>>> Please keep that in mind when looking at FIXME/TODO/XXX comments.
>>>
>>> Initially, MHDP driver was developed as a DRM bridge driver and was planed to
>>> be placed in drivers/gpu/drm/bridge/mhdp.c.  However, there was already
>>> a driver for Cadence's DP controller developed by RockChip, but that driver
>>> uses the different DRM framework and looks like a part of a bigger system.
>>> Both controllers (including firmware) are quite different internally
>>> (MST/FEC/DSC support, link training done by driver, additional commands, IRQ's
>>> etc.) but they have similar register map, except for Framer/Streamer (which is
>>> noticeably different), so they appear similar.
>>>
>>> The following patches contain:
>>> - Moving common code to drivers/gpu/drm/bridge/cdns-mhdp-common.* and
>>>   modifying it a bit (mostly new prefixes for functions and data types) so it
>>>   can be used by two, higher level, drivers.
>>> - Modifying existing RockChip's DP driver to use the common code after changes
>>>   made to it (use the new cdns_mhdp_device structure and new function names).
>>> - Modifying DRM helpers a bit. Some are required for new driver, some are
>>>   updates from DP 1.2 to 1.3 or 1.4.
>>> - Adding documentation for device tree bindings.
>>> - Adding preliminary Cadence DPI/DP bridge driver.
>>>
>>> Some of the things that will be added later on include (but are not limited
>>> to):
>>> - DSC support
>>> - FEC support
>>> - HDCP support
>> 
>> A few random comments/questions after a quick look at the patches.
>> 
>> The names of the source files and the kernel Kconfig are only about
>> "Cadence DP". But the DT bindings is for cdns,mhdp8546, and the
>> resulting module file is mhdp8546.ko. I think more consistency here
>> would be good.
>> 
>> I presume the part number (or family? are there other similar parts with
>> similar part numbers?) is relevant, so it should be in the Kconfig
>> option and help text, and probably in the file names too. The module
>> name should have "cdns" prefix there, similar to the source files and
>> the cdns-dsi.ko.
>> 
>> Or maybe the same driver will handle all Cadence DP parts, in which case
>> generic filenames are fine, but then the resulting kernel module should
>> also be just "cdns-mhdp.ko".
We would be very happy to have a separate driver for mhdp8546 instead of
mixing it with Rockchip's driver. Unfortunately we need to have one driver
for both IP's, so I'll just change mhdp8546.ko to cdns-mhdp.ko. (Unless,
maintainers give us a green light for the dedicated driver?)
>> 
>> I see some audio functions in the code, but it's not mentioned in the DT
>> bindings. I'm not an audio guy, but the display bridges with audio
>> support I have seen have had DT bindings for the audio source too. Is
>> audio supported in the current driver?
>> 
As far as I know, audio is working, but I can't double check it right now
due to heavy load on my back right now. I'll try to find time for that in the
next week. But, like I've mentioned earlier I don't see a reason why audio
code (that exists in the mainline) wouldn't work.
I'm not an audio guy myself. When updating dt bindings I took the dt bindings
for other bridges as an example, but I did not see any bindings for audio in
them. I didn't check all of them. Can you please let me know which bridges
are you were referring to? In our test environment we didn't use any audio
driver as audio generator is so simple that it is part of the register space
of IP and we didn't even need to include this device in .dts file. Audio (like
the video) is configured and enabled by code that is not included in the patch
(for obvious reasons).
So, in short, yes, driver supports audio and we will include description for
audio input port in the dt bindings.
>>  Tomi
>> 
>
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Regards,
Damian

-----Original Message-----
From: Tomi Valkeinen <tomi.valkeinen@ti.com> 
Sent: Wednesday, March 20, 2019 10:34
To: Damian Kos <dkos@cadence.com>
Cc: David Airlie <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Andrzej Hajda <a.hajda@samsung.com>; Laurent Pinchart <Laurent.pinchart@ideasonboard.com>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard <maxime.ripard@bootlin.com>; Sean Paul <sean@poorly.run>; Sandy Huang <hjc@rock-chips.com>; Heiko Stübner <heiko@sntech.de>; dri-devel@lists.freedesktop.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org; jbergsagel@ti.com; quentin.schulz@bootlin.com; Piotr Sroka <piotrs@cadence.com>; Rafal Ciepiela <rafalc@cadence.com>
Subject: Re: [PATCH v7 0/4] drm: add support for Cadence MHDP DPI/DP bridge.

EXTERNAL MAIL


Damian, ping.

On 31/01/2019 14:08, Tomi Valkeinen wrote:
> Hi,
> 
> On 30/01/2019 13:03, Damian Kos wrote:
>> Hello!
>>
>> This is the series of patches that will add support for the Cadence's DPI/DP
>> bridge. Please note that this is a preliminary version of the driver and there
>> will be more patches in the future with updates, fixes and improvements.
>> Please keep that in mind when looking at FIXME/TODO/XXX comments.
>>
>> Initially, MHDP driver was developed as a DRM bridge driver and was planed to
>> be placed in drivers/gpu/drm/bridge/mhdp.c.  However, there was already
>> a driver for Cadence's DP controller developed by RockChip, but that driver
>> uses the different DRM framework and looks like a part of a bigger system.
>> Both controllers (including firmware) are quite different internally
>> (MST/FEC/DSC support, link training done by driver, additional commands, IRQ's
>> etc.) but they have similar register map, except for Framer/Streamer (which is
>> noticeably different), so they appear similar.
>>
>> The following patches contain:
>> - Moving common code to drivers/gpu/drm/bridge/cdns-mhdp-common.* and
>>   modifying it a bit (mostly new prefixes for functions and data types) so it
>>   can be used by two, higher level, drivers.
>> - Modifying existing RockChip's DP driver to use the common code after changes
>>   made to it (use the new cdns_mhdp_device structure and new function names).
>> - Modifying DRM helpers a bit. Some are required for new driver, some are
>>   updates from DP 1.2 to 1.3 or 1.4.
>> - Adding documentation for device tree bindings.
>> - Adding preliminary Cadence DPI/DP bridge driver.
>>
>> Some of the things that will be added later on include (but are not limited
>> to):
>> - DSC support
>> - FEC support
>> - HDCP support
> 
> A few random comments/questions after a quick look at the patches.
> 
> The names of the source files and the kernel Kconfig are only about
> "Cadence DP". But the DT bindings is for cdns,mhdp8546, and the
> resulting module file is mhdp8546.ko. I think more consistency here
> would be good.
> 
> I presume the part number (or family? are there other similar parts with
> similar part numbers?) is relevant, so it should be in the Kconfig
> option and help text, and probably in the file names too. The module
> name should have "cdns" prefix there, similar to the source files and
> the cdns-dsi.ko.
> 
> Or maybe the same driver will handle all Cadence DP parts, in which case
> generic filenames are fine, but then the resulting kernel module should
> also be just "cdns-mhdp.ko".
> 
> I see some audio functions in the code, but it's not mentioned in the DT
> bindings. I'm not an audio guy, but the display bridges with audio
> support I have seen have had DT bindings for the audio source too. Is
> audio supported in the current driver?
> 
>  Tomi
> 

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki