Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221220094055.3011916-1-milkfafa@gmail.com> <20221220094055.3011916-8-milkfafa@gmail.com>
 <078aa497-e441-ef3d-c206-de23ca2658c9@collabora.com>
In-Reply-To: <078aa497-e441-ef3d-c206-de23ca2658c9@collabora.com>
From:   Kun-Fa Lin <milkfafa@gmail.com>
Date:   Thu, 22 Dec 2022 10:23:32 +0800
Message-ID: <CADnNmFpjrRy2J5mrFnp6JRiY5W=Xot83PV_JMuBWMP-uf63Rig@mail.gmail.com>
Subject: Re: [PATCH v9 7/7] media: nuvoton: Add driver for NPCM video capture
 and encode engine
To:     Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc:     mchehab@kernel.org, hverkuil-cisco@xs4all.nl,
        avifishman70@gmail.com, tmaimon77@gmail.com, tali.perry1@gmail.com,
        venture@google.com, yuenn@google.com, benjaminfair@google.com,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        devicetree@vger.kernel.org, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org,
        kwliu@nuvoton.com, kflin@nuvoton.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Andrzej,

Thanks for the review.

> > +static void npcm_video_ece_set_fb_addr(struct npcm_video *video, u32 buffer)
>
> static inline void?
>

> > +static void npcm_video_ece_set_enc_dba(struct npcm_video *video, u32 addr)
>
> ditto

> > +static void npcm_video_ece_clear_rect_offset(struct npcm_video *video)
>
> ditto

> > +static int npcm_video_ece_init(struct npcm_video *video)
>
> static inline int? But...
>
> > +{
> > +     npcm_video_ece_ip_reset(video);
> > +     npcm_video_ece_ctrl_reset(video);
> > +
> > +     return 0;
>
> ...the return value is not inspected by the only caller anyway, so why not
>
> static inline void?

OK, I'll declare these functions as static inline void.

> > +static int npcm_video_ece_stop(struct npcm_video *video)
> > +{
> > +     struct regmap *ece = video->ece.regmap;
> > +
> > +     regmap_update_bits(ece, ECE_DDA_CTRL, ECE_DDA_CTRL_ECEEN, 0);
> > +     regmap_update_bits(ece, ECE_DDA_CTRL, ECE_DDA_CTRL_INTEN, 0);
> > +     regmap_update_bits(ece, ECE_HEX_CTRL, ECE_HEX_CTRL_ENCDIS,
> > +                        ECE_HEX_CTRL_ENCDIS);
> > +     npcm_video_ece_clear_rect_offset(video);
> > +
> > +     return 0;
>
> Nobody inspects this return value. Either void, or check the return value
> at call site and handle a non-zero failure.

OK, will change to void.

> > +static unsigned int npcm_video_get_rect_count(struct npcm_video *video)
> > +{
> > +     struct list_head *head, *pos, *nx;
> > +     struct rect_list *tmp;
> > +     unsigned int count;
>
>         unsigned int count = 0;
>
> otherwise if the below condition is not met, ...
> > +
> > +     if (video->list && video->rect) {
> > +             count = video->rect[video->vb_index];
> > +             head = &video->list[video->vb_index];
> > +
> > +             list_for_each_safe(pos, nx, head) {
> > +                     tmp = list_entry(pos, struct rect_list, list);
> > +                     list_del(&tmp->list);
> > +                     kfree(tmp);
> > +             }
>
> why does a function whose name implies merely getting a number actually
> remove all elements from some list? count equals video->rect[video->vb_index];
> and everthing else looks like a(n indented?) side effect. This should be
> somehow commented in the code.
>
> > +     }
> > +
> > +     return count;
>
> ... an undefined number is returned
>
> Which makes me wonder if the compiler is not warning about using a possibly
> uninitialized value.

You are right, this is not the right place to remove the rect_list.
It makes more sense to remove the list right after the associated
video buffer gets dequeued.
I'll do that change.

> > +static int npcm_video_capres(struct npcm_video *video, u32 hor_res,
> > +                          u32 vert_res)
> > +{
> > +     struct regmap *vcd = video->vcd_regmap;
> > +     u32 res, cap_res;
> > +
> > +     if (hor_res > MAX_WIDTH || vert_res > MAX_HEIGHT)
> > +             return -EINVAL;
> > +
> > +     res = FIELD_PREP(VCD_CAP_RES_VERT_RES, vert_res) |
> > +           FIELD_PREP(VCD_CAP_RES_HOR_RES, hor_res);
> > +
> > +     regmap_write(vcd, VCD_CAP_RES, res);
> > +     regmap_read(vcd, VCD_CAP_RES, &cap_res);
> > +
> > +     if (cap_res != res)
> > +             return -EINVAL;
> > +
> > +     return 0;
>
> The return value is not handled by the caller

> > +static int npcm_video_gfx_reset(struct npcm_video *video)
> > +{
> > +     struct regmap *gcr = video->gcr_regmap;
> > +
> > +     regmap_update_bits(gcr, INTCR2, INTCR2_GIRST2, INTCR2_GIRST2);
> > +
> > +     npcm_video_vcd_state_machine_reset(video);
> > +
> > +     regmap_update_bits(gcr, INTCR2, INTCR2_GIRST2, 0);
> > +
> > +     return 0;
>
> Never inspected by callers

> > +static int npcm_video_command(struct npcm_video *video, u32 value)
> > +{
> > +     struct regmap *vcd = video->vcd_regmap;
> > +     u32 cmd;
> > +
> > +     regmap_write(vcd, VCD_STAT, VCD_STAT_CLEAR);
> > +
> > +     regmap_read(vcd, VCD_CMD, &cmd);
> > +     cmd |= FIELD_PREP(VCD_CMD_OPERATION, value);
> > +
> > +     regmap_write(vcd, VCD_CMD, cmd);
> > +     regmap_update_bits(vcd, VCD_CMD, VCD_CMD_GO, VCD_CMD_GO);
> > +     video->op_cmd = value;
> > +
> > +     return 0;
>
> Never inspected by caller

> > +static int npcm_video_start_frame(struct npcm_video *video)
> > +{
>
> One of the callers ignores the return value, but not the other. Why?

These problems will be addressed in the next patch. Thank you.

Regards,
Marvin