Messages in this thread | | | Date | Tue, 3 Jan 2023 13:48:23 +0100 | Subject | Re: [PATCH v10 7/7] media: nuvoton: Add driver for NPCM video capture and encode engine | From | Paul Menzel <> |
| |
Dear Kun-Fa,
Am 29.12.22 um 09:55 schrieb Kun-Fa Lin:
>>> Add driver for Video Capture/Differentiation Engine (VCD) and Encoding >>> Compression Engine (ECE) present on Nuvoton NPCM SoCs. The VCD can >>> capture and differentiate video data from digital or analog sources, >> >> “differentiate video data” sounds uncommon to me. Am I just ignorant or >> is there a better term? > > How about "The VCD can capture a frame from digital video input and > compare two frames in memory, then the ECE will compress the frame > data into HEXTITLE format", is it better?
Yes, I prefer your suggestion.
>> Wich VNC viewer and version? > > I used RealVNC version 6.21.1109 to test. > Do I have to add this information in the commit message?
I do not think there are rules, but I prefer to have the test environment and procedure information in the commit message in case there are problems, and you want to reproduce things.
>> Maybe also paste the new dev_ log messages you get from one boot. > > Do you mean dev_info/dev_debug messages of the driver? > If yes, I get these messages from one boot (only dev_info will be > printed in default): > > npcm-video f0810000.video: assigned reserved memory node framebuffer@0x33000000 > npcm-video f0810000.video: NPCM video driver probed
Yes, that is what I meant. Maybe even the debug messages.
>> It’d be great if you noted the datasheet name and revision. > > I can note the datasheet name and revision in the commit message but > can't provide the file link because it is not public. > Is it ok with you?
Yes, that would be ok with me.
>>> +static unsigned int npcm_video_ece_get_ed_size(struct npcm_video *video, >>> + u32 offset, u8 *addr) >>> +{ >>> + struct regmap *ece = video->ece.regmap; >>> + u32 size, gap, val; >> >> Using a fixed size type for variables not needing is, is actually not an >> optimization [1]. It’d be great, if you went over the whole change-set >> to use the non-fixed types, where possible. (You can also check the >> difference with `scripts/bloat-o-meter`. > > So what I have to do is replace "u8/u16/u32" with "unsigned int" for > generic local variables as much as possible. > Is my understanding correct?
Yes, I would say so.
>>> +MODULE_AUTHOR("Joseph Liu<kwliu@nuvoton.com>"); >>> +MODULE_AUTHOR("Marvin Lin<kflin@nuvoton.com>"); >> >> Please add a space before the <. >> >>> +MODULE_DESCRIPTION("Driver for Nuvoton NPCM Video Capture/Encode Engine"); >>> +MODULE_LICENSE("GPL"); >> >> Not GPL v2? > > I'll correct them in the next patch.
Awesome.
Kind regards,
Paul
| |