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
