linux-kernel - Re: [PATCH v10 7/7] media: nuvoton: Add driver for NPCM video capture and encode engine

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 29 Dec 2022 16:55:50 +0800
From:   Kun-Fa Lin <milkfafa@...il.com>
To:     Paul Menzel <pmenzel@...gen.mpg.de>
Cc:     mchehab@...nel.org, hverkuil-cisco@...all.nl,
        avifishman70@...il.com, tmaimon77@...il.com, tali.perry1@...il.com,
        venture@...gle.com, yuenn@...gle.com, benjaminfair@...gle.com,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        andrzej.p@...labora.com, kwliu@...oton.com,
        devicetree@...r.kernel.org, openbmc@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org, kflin@...oton.com,
        linux-media@...r.kernel.org
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@...3000000
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@...oton.com>");
> > +MODULE_AUTHOR("Marvin Lin<kflin@...oton.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