[U-Boot] [PATCH 3/5] rockchip: video: rk3399: enable HDMI output (from the rk_vop) for the RK3399

[Dr. Philipp Tomsich]

Hi Simon,

> On 30 Apr 2017, at 05:49, Simon Glass <sjg at chromium.org> wrote:
> 
> Hi Philipp,
> 
> On 28 April 2017 at 09:53, Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich at theobroma-systems.com>> wrote:
>> This commit enables RK3399 support for HDMI through the following
>> changes:
>> - adds a driverdata structure to mirror some subtle version
>>  differences between the RK3399 VOPs and those in the RK3288
>>  (e.g. the pin-polarity configuration)
>> - configures the VOP to output 32bpp for HDMI
>> - handles whether a VOP can output 10BIT data or not (i.e. RGBaaa vs. RGB888)
>>  using the driverdata structure
>> 
>> And as we touch this file anyway, we also increase the size of the
>> framebuffer to a slightly overzealous 4K2K at 32bpp.
>> 
>> Signed-off-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
>> ---
>> 
>> arch/arm/include/asm/arch-rockchip/vop_rk3288.h |  11 ++
>> drivers/video/rockchip/rk_vop.c                 | 180 ++++++++++++++++++++----
>> 2 files changed, 161 insertions(+), 30 deletions(-)
> 
> I'd like to somehow keep the SoC-specific code out of this driver.
> You've done a nice job separating it out, but I wonder if we can go a
> bit further.
> 
> I'm thinking of perhaps having two vop drivers, one for rk3288 and one
> for rk3399. They can share most of the operations (e.g. bind()) which
> can stay in the existing rk_vop.c file. For probe() you can rename the
> existing probe() function to something like rockchip_vop_probe(), and
> pass it the device and a rkvop_driverdata *.
> 
> Does that make sense? Then when we add more chips we'll have a small
> extra file with the SoC-specific functionality.

I had considered (for any future work on this) to go into the opposite direction
and to port drivers/gpu/drm/rockchip/rockchip_vop_reg.[ch] from Linux (and
possibly even drivers/gpu/drm/rockchip/rockchip_drm_vop.c … those treat
the various variants (3288,3328,  3366, 3368, 3399-lit, 3399-big) as a single
hardware block that has a different version and sometimes has the capability
of supporting optional features (e.g. 10bit RGB output)

Instead of splitting things up it would thus put them into a single driver and
then use driverdata to model all the differences.  And to make things easier
for the long-term maintenance (and avoid mistakes in the first place), I would
rather try and reuse (large parts of) rockchip_vop_reg.[ch] verbatim in U-Boot.

Keeping things aligned with the Linux driver would be my preferred long-term
solution, if I can get a consensus for it…

Cheers,
Philipp.