Re: [RFC PATCH V0 7/7] [media] platform: mtk-isp: Add Mediatek ISP Pass 1 driver

[Jungo Lin <jungo.lin@mediatek.com>]

On Thu, 2019-03-21 at 12:48 +0900, Tomasz Figa wrote:
> On Tue, Mar 12, 2019 at 5:16 PM Jungo Lin <jungo.lin@mediatek.com> wrote:
> >
> > On Thu, 2019-03-07 at 19:04 +0900, Tomasz Figa wrote:
> [snip]
> > > > +struct mtk_cam_mem2mem2_device {
> > > > +       const char *name;
> > > > +       const char *model;
> > >
> > > For both of the fields above, they seem to be always
> > > MTK_CAM_DEV_P1_NAME, so we can just use the macro directly whenever
> > > needed. No need for this indirection.
> > >
> >
> > OK. These two fields will be removed in next patch.
> >
> > > > +       struct device *dev;
> > > > +       int num_nodes;
> > > > +       struct mtk_cam_dev_video_device *nodes;
> > > > +       const struct vb2_mem_ops *vb2_mem_ops;
> > >
> > > This is always "vb2_dma_contig_memops", so it can be used directly.
> > >
> >
> > Ditto.
> >
> > > > +       unsigned int buf_struct_size;
> > >
> > > This is always sizeof(struct mtk_cam_dev_buffer), so no need to save
> > > it in the struct.
> > >
> >
> > Ditto.
> >
> > > > +       int streaming;
> > > > +       struct v4l2_device *v4l2_dev;
> > > > +       struct media_device *media_dev;
> > >
> > > These 2 fields are already in mtk_cam_dev which is a superclass of
> > > this struct. One can just access them from there directly.
> > >
> >
> > Ditto.
> >
> > > > +       struct media_pipeline pipeline;
> > > > +       struct v4l2_subdev subdev;
> > >
> > > Could you remind me what was the media topology exposed by this
> > > driver? This is already the second subdev I spotted in this patch,
> > > which looks strange.
> > >
> >
> >
> > For sub-device design, we will remove the sub-device for CIO and keep
> > only one sub-device for ISP driver in next patch. We will also provide
> > the media topology in RFC v1 patch to clarify.
> >
> > > > +       struct media_pad *subdev_pads;
> > > > +       struct v4l2_file_operations v4l2_file_ops;
> > > > +       const struct file_operations fops;
> > > > +};
> > >
> > > Given most of the comments above, it looks like the remaining useful
> > > fields in this struct could be just moved to mtk_cam_dev, without the
> > > need for this separate struct.
> > >
> >
> > This is the final revision for these two structures.
> > Do you suggest to merge it to simplify?
> >
> > struct mtk_cam_mem2mem2_device {
> >         struct mtk_cam_video_device *nodes;
> >         struct media_pipeline pipeline;
> >         struct v4l2_subdev subdev;
> >         struct media_pad *subdev_pads;
> > };
> >
> > struct mtk_cam_dev {
> >         struct platform_device *pdev;
> >         struct mtk_cam_video_device     mem2mem2_nodes[MTK_CAM_DEV_NODE_MAX];
> >         struct mtk_cam_mem2mem2_device mem2mem2;
> >         struct mtk_cam_io_connection cio;
> >         struct v4l2_device v4l2_dev;
> >         struct media_device media_dev;
> >         struct mtk_cam_ctx ctx;
> >         struct v4l2_async_notifier notifier;
> > };
> >
> 
> I feel like there is not much benefit in having this split. Similarly,
> I'm not sure if there is a reason to have separate structs for
> mtk_cam_io_connection and mtk_cam_ctx.
> 
> (Sorry, missed this one in previous reply.)
> 
> Best regards,
> Tomasz

Ok, agree your comment.
We will remove both mtk_cam_io_connection and mtk_cam_ctx and
merge those fields into mtk_cam_dev.

Thanks for your suggestion.

Best regards,


Jungo



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel