Messages in this thread | | | From | Kun-Fa Lin <> | Date | Thu, 29 Dec 2022 16:55:50 +0800 | Subject | Re: [PATCH v10 7/7] media: nuvoton: Add driver for NPCM video capture and encode engine |
| |
Hi Paul,
Thanks for the review.
> > 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?
> 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?
> 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
> 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?
> > +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?
> > +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.
Regards, Marvin
| |