On Sat, 2 Aug 2014 17:28:45 +0200 Luc Verhaegen <[email protected]> wrote:
> On Sat, Aug 02, 2014 at 06:03:40PM +0300, Siarhei Siamashka wrote: > > On Sat, 2 Aug 2014 16:06:09 +0200 > > Luc Verhaegen <[email protected]> wrote: > > > > > This adds a fixed mode hdmi driver (lcd to be added in future) for the > > > sunxi platform. Current config is such that 8MB is shaved off at the top > > > of the RAM. Simplefb support is available for kernels that know how to > > > use it. > > > > > > Signed-off-by: Luc Verhaegen <[email protected]> > > > --- > > > arch/arm/include/asm/arch-sunxi/sunxi_display.h | 21 + > > > board/sunxi/board.c | 14 + > > > drivers/video/Makefile | 1 + > > > drivers/video/sunxi_display.c | 640 > > > +++++++++++++++++++++++ > > > include/configs/sunxi-common.h | 36 ++ > > > 5 files changed, 712 insertions(+), 0 deletions(-) > > > create mode 100644 arch/arm/include/asm/arch-sunxi/sunxi_display.h > > > create mode 100644 drivers/video/sunxi_display.c > > > > Thanks a lot. This is a really very needed feature and a major > > milestone for the sunxi community. I know that this work had > > been started a bit late and you had very little time to port > > this code from the kms sunxi, so there are still some rough > > edges. This makes it even more impressive. > > What rough edges? An forgotten struct member, probably split the patch > up in 2 (second adding simplefb). Env modes are not a target for me at > this time. Well, I did not have time to look at the patch in details yet. But I guess, quite a number of nitpicks from various people are to be expected. For example, 1024x768 only needs 4M and not 8M. Also the devicetree experts might complain about something in the simplefb integration (Maxime? Emilio? anybody else?). Splitting the patch would be indeed quite reasonable. For example, the changes in 'sunxi-common.h' to activate the driver may be split into a separate patch in order to reduce the chance of merge conflicts. And also because they are likely to be handled by different custodians ('video' and 'sunxi'?). The PSCI was a good example: it had a generic ARM part and there also was a sunxi specific part. You don't seem to like how some parts of the u-boot video code are implemented. But that's not a code beauty contest. We just want the HDMI console for the debugging output in u-boot and also simplefb in the mainline linux kernel. The code in u-boot may be ugly as hell and ridden with legacy warts, however we don't have to care as long as it works and the u-boot maintainers are happy :) > checkpatch was of course run, but only meaningless checks remain. OK > Thanks for the praise though. > > > The patch applies without conflicts to the mainline u-boot after > > just taking the default environment from u-boot-sunxi: > > https://github.com/ssvb/u-boot-sunxi-dram/commit/a891813410b13e91 > > > > And it works. At least provides the hdmi console with the u-boot log > > messages. > > Ok. Thanks for looknig into this. So far I just quickly checked that it applies cleanly and does what it is expected to do. > > > > By the way, the merge window for u-boot v2014.10 is still open for ~13 > > hours according to http://www.denx.de/wiki/U-Boot/ReleaseCycle > > "The Merge Window for the next release (v2014.10) is open until > > Sat, Aug 02, 2014 23:59:59 EDT" > > 6.5h, but i have a social engagement. http://www.thetimenow.com/edt/eastern_daylight_time says that ~11.5 hours are still remaining until the deadline. Also http://www.denx.de/wiki/U-Boot/DevelopmentProcess says "Sometimes patches miss the the Merge Window slightly - say by few hours or even a day. Patch acceptance is not as critical as a financial transaction, or such. So if there is such a slight delay, the custodian is free to turn a blind eye and accept it anyway. The idea of the development process is to make it foreseeable, but not to slow down development." Now u-boot is about to move from the "Merge Window" into the "Twilight Time" stage. Would you mind if somebody else submitted the patch to the mainline u-boot on your behalf? Either way, it's a standalone driver and seems to be really easy to apply to any u-boot release. And we are still going to have a sunxi-specific u-boot tree, at least with the extra sunxi-3.4 kernel compatibility tweaks :) However there are still some major features missing in the mainline u-boot: NAND, better compatibility with USB keyboards, maybe EDID for the video driver, suspend to RAM, ... In other words, a lot of work to do for the next u-boot merge window. So reducing the number of non-mainlined patches in the current u-boot release should make the work easier later. > > There might be still enough time to submit the v1 version of the > > patch to the u-boot mailing list: > > http://lists.denx.de/mailman/listinfo/u-boot > > > > Adding 'video' and 'sunxi' custodians to CC would be necessary if > > you decide to go this route: > > http://www.denx.de/wiki/U-Boot/Custodians > > > > Also it may make sense to get rid of the checkpatch.pl complaints > > before the patch submission: > > > > [...] > > > > CHECK: No space is necessary after a cast > > #605: FILE: drivers/video/sunxi_display.c:510: > > +{ > > + void *composer = (void *) SUNXI_DE_BE0_BASE; > > > > CHECK: No space is necessary after a cast > > #606: FILE: drivers/video/sunxi_display.c:511: > > + void *composer = (void *) SUNXI_DE_BE0_BASE; > > + void *lcdc = (void *) SUNXI_LCD0_BASE; > > > > CHECK: No space is necessary after a cast > > #607: FILE: drivers/video/sunxi_display.c:512: > > + void *lcdc = (void *) SUNXI_LCD0_BASE; > > + void *hdmi = (void *) SUNXI_HDMI_BASE; > > > > total: 0 errors, 0 warnings, 17 checks, 739 lines checked > > > > NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX > > MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE > > USLEEP_RANGE > > > > 0001-video-add-cfb-console-driver-for-sunxi.patch has style problems, > > please review. > > > > If any of these errors are false positives, please report > > them to the maintainer, see CHECKPATCH in MAINTAINERS. > > Those checks are all false positives. I like the space after the cast > (void *), it is easier on the eye imho. But this is a very finnicky > matter of taste. Then there are the camelcase checks, etc. OK. The checkpatch.pl was just saying "has style problems, please review" and I'm not that sensitive to the coding style to have a really strong opinion about the extra spaces :) -- Best regards, Siarhei Siamashka -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
