> On 03 May 2017, at 05:00, Simon Glass <s...@chromium.org> wrote: > > Hi Philipp, > > On 30 April 2017 at 06:31, Dr. Philipp Tomsich > <philipp.toms...@theobroma-systems.com > <mailto:philipp.toms...@theobroma-systems.com>> wrote: >> Hi Simon, >> >> On 30 Apr 2017, at 05:49, Simon Glass <s...@chromium.org> wrote: >> >> Hi Philipp, >> >> On 28 April 2017 at 09:53, Philipp Tomsich >> <philipp.toms...@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.toms...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
