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

From: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: "Damian Kos" <dkos@cadence.com>,
	"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
Cc: <jbergsagel@ti.com>, <quentin.schulz@bootlin.com>,
	<piotrs@cadence.com>, <rafalc@cadence.com>
Subject: Re: [PATCH v7 0/4] drm: add support for Cadence MHDP DPI/DP bridge.
Date: Thu, 31 Jan 2019 14:08:18 +0200	[thread overview]
Message-ID: <0f11a4d8-d576-5af8-b7f4-afe3ff26678f@ti.com> (raw)
In-Reply-To: <1548846209-16406-1-git-send-email-dkos@cadence.com>

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