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

[Dr. Philipp Tomsich]

> On 03 May 2017, at 05:00, Simon Glass <sjg at chromium.org> wrote:
> 
> Hi Philipp,
> 
> On 30 April 2017 at 06:31, Dr. Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich at theobroma-systems.com>> wrote:
>> 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> 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…
> 
> I do understand this desire, but I worry that we will end up with
> monolithic drivers where adding video support will cost a lot in code
> space. Linux has essentially unlimited code space so the trade-offs
> are different.
> 
> If you do end up porting these things more directly from Linux then
> I'd like to see how things can be broken up so that we can scale the
> code size functionality a bit.
> 
> You are effectively creating a new API layer within the driver to
> handle hardware differences. I really like that approach, but I feel
> that putting all the implementations in this one file is unwieldy.
> 
> Having said that, from the example you gave I doubt there is a very
> large difference in code size?

In fact I started to look into the changes in more detail yesterday and already
started to cut this up into individual drivers along your suggestion: on second
look reusing the Linux code does not conflict the approach you suggested, as
I can supply the register layout differences from the SoC-specific “mini-drivers”.

Regards,
Philipp.