Hi, Tomasz, Thanks for the review again. > -----Original Message----- > From: Tomasz Figa [mailto:tfiga@xxxxxxxxxxxx] > Sent: Thursday, April 26, 2018 12:15 AM > To: Zhi, Yong <yong.zhi@xxxxxxxxx> > Cc: Linux Media Mailing List <linux-media@xxxxxxxxxxxxxxx>; Sakari Ailus > <sakari.ailus@xxxxxxxxxxxxxxx>; Mani, Rajmohan > <rajmohan.mani@xxxxxxxxx>; Toivonen, Tuukka > <tuukka.toivonen@xxxxxxxxx>; Hu, Jerry W <jerry.w.hu@xxxxxxxxx>; Zheng, > Jian Xu <jian.xu.zheng@xxxxxxxxx> > Subject: Re: [PATCH v6 12/12] intel-ipu3: Add imgu top level pci device > driver > > Hi Yong, > > On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi <yong.zhi@xxxxxxxxx> wrote: > [snip] > > +static int imgu_video_nodes_init(struct imgu_device *imgu) { > > + struct v4l2_pix_format_mplane *fmts[IPU3_CSS_QUEUES] = { NULL }; > > + struct v4l2_rect *rects[IPU3_CSS_RECTS] = { NULL }; > > + unsigned int i; > > + int r; > > + > > + imgu->buf_struct_size = sizeof(struct imgu_buffer); > > + > > + for (i = 0; i < IMGU_NODE_NUM; i++) { > > + imgu->nodes[i].name = imgu_node_map[i].name; > > + imgu->nodes[i].output = i < IMGU_QUEUE_FIRST_INPUT; > > + imgu->nodes[i].immutable = false; > > + imgu->nodes[i].enabled = false; > > + > > + if (i != IMGU_NODE_PARAMS && i != IMGU_NODE_STAT_3A) > > + fmts[imgu_node_map[i].css_queue] = > > + &imgu->nodes[i].vdev_fmt.fmt.pix_mp; > > + atomic_set(&imgu->nodes[i].sequence, 0); > > + } > > + > > + /* Master queue is always enabled */ > > + imgu->nodes[IMGU_QUEUE_MASTER].immutable = true; > > + imgu->nodes[IMGU_QUEUE_MASTER].enabled = true; > > + > > + r = ipu3_v4l2_register(imgu); > > + if (r) > > + return r; > > + > > + /* Set initial formats and initialize formats of video nodes */ > > + rects[IPU3_CSS_RECT_EFFECTIVE] = &imgu->rect.eff; > > + rects[IPU3_CSS_RECT_BDS] = &imgu->rect.bds; > > + ipu3_css_fmt_set(&imgu->css, fmts, rects); > > + > > + /* Pre-allocate dummy buffers */ > > + r = imgu_dummybufs_preallocate(imgu); > > + if (r) { > > + dev_err(&imgu->pci_dev->dev, > > + "failed to pre-allocate dummy buffers (%d)", r); > > + imgu_dummybufs_cleanup(imgu); > > No need to call ipu3_v4l2_unregister() here? > > (That's why I keep suggesting use of single return error path with labels > named after the first cleanup step that needs to be done, as it makes it > easier to spot such mistakes.) > Good catch, suggestion taken :) Maybe I should move the imgu_dummybufs_preallocate() out from imgu_video_nodes_init(). > > + return r; > > + } > > + > > + return 0; > > +} > > + > > +static void imgu_video_nodes_exit(struct imgu_device *imgu) { > > + imgu_dummybufs_cleanup(imgu); > > + ipu3_v4l2_unregister(imgu); > > +} > > Best regards, > Tomasz
