On 03 May 2017, at 05:00, Simon Glass sjg@chromium.org wrote:
Hi Philipp,
On 30 April 2017 at 06:31, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com mailto:philipp.tomsich@theobroma-systems.com> wrote:
Hi Simon,
On 30 Apr 2017, at 05:49, Simon Glass sjg@chromium.org wrote:
Hi Philipp,
On 28 April 2017 at 09:53, Philipp Tomsich philipp.tomsich@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@32bpp.
Signed-off-by: Philipp Tomsich philipp.tomsich@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.